Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vultr fix #397

Merged
merged 5 commits into from
Aug 14, 2024
Merged

Vultr fix #397

merged 5 commits into from
Aug 14, 2024

Conversation

jokestax
Copy link
Contributor

Description

Implemented a timout error as cluster deployment takes some time to get deployed,otherwise it wont wait for the cluster and returns an connection refused error

How to test

kubefirst beta vultr create --alerts-email <ALERT_MAIL> --github-org <ORG_NAME> --domain-name <DOMAIN_NAME> --gitops-template-branch patch-1 --cluster-name <CLUSTER_NAME> --gitops-template-url https://github.com/Seven45/gitops-template

Copy link
Member

@patrickdappollonio patrickdappollonio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get some more context here, why is it that the watch process fails? Is it because the pod hasn't been created there yet so you get 404s?

internal/k8s/exec.go Outdated Show resolved Hide resolved
internal/k8s/exec.go Outdated Show resolved Hide resolved
Copy link
Member

@patrickdappollonio patrickdappollonio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way is good with me so approving this but I think we don't need the extra difficulty if we're going to end up passing a single label anyways.

Thank you, this looks great!

internal/k8s/exec.go Outdated Show resolved Hide resolved
@jokestax jokestax self-assigned this Aug 12, 2024
@jokestax jokestax added the bug Something isn't working label Aug 12, 2024
@fharper
Copy link
Contributor

fharper commented Aug 12, 2024

@patrickdappollonio you also tested this PR since you approved it, not just checked the code? Just want to be sure as I won't test it since you approved.

@patrickdappollonio
Copy link
Member

We made a unit test, but I don't think @jokestax shared it. Might need the human test instead.

@jokestax
Copy link
Contributor Author

It needs a manual test :)

@fharper
Copy link
Contributor

fharper commented Aug 13, 2024

All PRs need to be tested in a way or another (often E2E) before we approved them, not just code reviews. I assumed it was done when approved, but no worries, I'll try to find time today to test it also :)

@fharper
Copy link
Contributor

fharper commented Aug 14, 2024

When running it, I got this error.

CleanShot 2024-08-14 at 10 22 11@2x

Not much more information in the logs.

CleanShot 2024-08-14 at 10 23 15@2x

Any idea if it's a code 18 or something in the code?

@jokestax
Copy link
Contributor Author

Ohh when you are trying to run api locally there is new change done ,we have export K1_CONSOLE_REMOTE_URL="http://localhost:3000"

@fharper
Copy link
Contributor

fharper commented Aug 14, 2024

Oh OK, we should add this to the . env file if not already done. I will give it another try, thanks.

@patrickdappollonio
Copy link
Member

@mrsimonemms this might be good to have in the .devcontainer so instead of having people run the long command I see in @fharper's screenshot, it could simply be run through the container.

@patrickdappollonio
Copy link
Member

Oh wait, maybe that's kubefirst, not kubefirst-api, my bad.

Copy link
Contributor

@fharper fharper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this PR, and I wasn't able to get a successful run: it's stuck at wrapping up", but I don't think it's related to this fix. I didn't get the timeout issue on Argo CD or Vault, so I think we can merge.

@jokestax jokestax merged commit 0559e3b into main Aug 14, 2024
2 checks passed
@jokestax jokestax deleted the vultr-fix branch August 14, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants