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

fixup! Missing LINODE_TOKEN error message #82

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

cbzzz
Copy link
Contributor

@cbzzz cbzzz commented Feb 6, 2024

Description

Addresses: #76 (comment)

Notes

I'm not sure we should be using a global LINODE_TOKEN for the operator (and all it's managed workload clusters) here. We should allow workload clusters to specify the Linode account in which to provision its resources.

Copy link
Contributor

@mhmxs mhmxs left a comment

Choose a reason for hiding this comment

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

It makes sense, lots of other project uses setupLog in the same way.

@eljohnson92
Copy link
Collaborator

@mhmxs is it expected that the e2e-test failure just because the test is coming from a fork?

@cbzzz
Copy link
Contributor Author

cbzzz commented Feb 6, 2024

@mhmxs It seems like the E2E test doesn't have the LINODE_TOKEN environment variable set. Is this expected? Thanks!

https://github.com/linode/cluster-api-provider-linode/actions/runs/7805739210/job/21292435053#step:7:346

@AshleyDumaine
Copy link
Contributor

@cbzzz I think you would need to set the LINODE_TOKEN secret on your fork for GHA to pass: https://github.com/linode/cluster-api-provider-linode/blob/main/.github/workflows/build_test_ci.yml#L139C35-L139C47

@mhmxs
Copy link
Contributor

mhmxs commented Feb 6, 2024

Is this expected?

@cbzzz I our current setup: Yes

@mhmxs
Copy link
Contributor

mhmxs commented Feb 6, 2024

set the LINODE_TOKEN

@cbzzz @AshleyDumaine keep in mind, that test execution should cost money. At the time of this we don't have any tests, but this should change soon: #80

@eljohnson92
Copy link
Collaborator

I think there is a larger discussion about when we want to run these e2e tests but I don't think that should be blocking on this PR. I'm good to merge as is if there are no other comments

@AshleyDumaine AshleyDumaine merged commit f1bc15e into linode:main Feb 6, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants