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

Use correct config for additional clusters #5034

Closed

Conversation

XenoAura
Copy link

@XenoAura XenoAura commented Jul 6, 2022

Fix: #5033

Description of the change

clientsetForConfig uses incorrect cluster config variable

svcConfig := *config
svcConfig.BearerToken = additionalCluster.ServiceToken
svcClientset, err = a.clientsetForConfig(config)

Benefits

Correct behavior for getting a list of namespaces for additional clusters

Possible drawbacks

Applicable issues

Additional information

@netlify
Copy link

netlify bot commented Jul 6, 2022

Deploy Preview for kubeapps-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 037a2c4
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/6304ad2aed462a0009c9a7af
😎 Deploy Preview https://deploy-preview-5034--kubeapps-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@vmwclabot
Copy link

@XenoAura, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@antgamdia
Copy link
Contributor

Thanks for the contribution @XenoAura ! LGTM, however, could you add a unit test so that we can avoid future regressions (we are planning to deprecate Kubeops and move the logic to the kubeapps-apis service) ?

Additionally, it seems the CircleCI checks are not passing (I mean, not failing, just that, sometimes, they don't get reported back, perhaps you need an empty commit or just merging main into your branch)

Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Forgot to actually +1 the changes 😅 , just waiting for the test to merge it.

@vmwclabot
Copy link

@XenoAura, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement.

@antgamdia
Copy link
Contributor

Let me update the branch with the latest changes (mainly to trigger the CI checks, which were somehow stuck). Also, the CLA bot complained about the signing process, please let us know if you are facing any issues with it. Feel free to reach out to us in Slack.

@vmwclabot
Copy link

@XenoAura, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement.

1 similar comment
@vmwclabot
Copy link

@XenoAura, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement.

@antgamdia
Copy link
Contributor

@XenoAura , is there anything I can help you with the CLA issues the bot is reporting? Or maybe with the unit test?

castelblanque pushed a commit that referenced this pull request Sep 2, 2022
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@castelblanque
Copy link
Collaborator

After removal of kubeops (#5253), parts of the enclosing code used by this PR have been migrated and thus can not be applied.
Changes have been reworked in #5286.
Thanks to @XenoAura for the contribution and creating the issue #5033.

castelblanque added a commit that referenced this pull request Sep 7, 2022
* Added logic from #5034

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Move the SA token to its own client getter

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Added missing error check

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Fix for return value

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Setting additional SA token dynamically

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Fix wrong command

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubeops cannot return a list of namespaces in additional clusters
4 participants