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 per-namespace app repositories by default #1604

Merged
merged 10 commits into from
Mar 26, 2020

Conversation

absoludity
Copy link
Contributor

Enables allowNamespaceDiscovery and reposPerNamespace to be on by default (as well as the invalidateCache which is required whenever we release a new Kubeapps version).

Updates the documentation for private app repos to match.

If we don't have any separate chart releases in the next few days, it might be good to land this and test it locally before releasing. Happy either way.

@absoludity absoludity requested a review from andresmgot March 25, 2020 04:57
@absoludity
Copy link
Contributor Author

@andresmgot I'll fix the conflict, but I'm not sure why this is failing on CI with the admin user not having access to fetch AppRepos in the kubeapps namespace? (see https://19293-107555654-gh.circle-artifacts.com/0/integration/reports/screenshots/creates-a-registry.png , which is from integration/use-cases/create-registry.js which logs in with ADMIN_TOKEN. I'll look more closely in the morning.

@andresmgot
Copy link
Contributor

andresmgot commented Mar 25, 2020

@andresmgot I'll fix the conflict, but I'm not sure why this is failing on CI with the admin user not having access to fetch AppRepos in the kubeapps namespace? (see https://19293-107555654-gh.circle-artifacts.com/0/integration/reports/screenshots/creates-a-registry.png , which is from integration/use-cases/create-registry.js which logs in with ADMIN_TOKEN. I'll look more closely in the morning.

The error could be misleading, if the component is receiving a forbidden error and the error message is not parse-able as a Forbidden error it shows the default required RBAC roles error that is the one you are seeing. Another possibility is that the admin user has RBAC permissions to manage apprepositories only in the kubeapps namespace (ref, since that namespace is now like a "global" namespace that can be also the error if we are requesting apprepositories in all namespaces.

@absoludity
Copy link
Contributor Author

@andresmgot I'll fix the conflict, but I'm not sure why this is failing on CI with the admin user not having access to fetch AppRepos in the kubeapps namespace? (see https://19293-107555654-gh.circle-artifacts.com/0/integration/reports/screenshots/creates-a-registry.png , which is from integration/use-cases/create-registry.js which logs in with ADMIN_TOKEN. I'll look more closely in the morning.

The error could be misleading, if the component is receiving a forbidden error and the error message is not parse-able as a Forbidden error it shows the default required RBAC roles error that is the one you are seeing. Another possibility is that the admin user has RBAC permissions to manage apprepositories only in the kubeapps namespace (ref, since that namespace is now like a "global" namespace that can be also the error if we are requesting apprepositories in all namespaces.

Thanks for the background. That helped me see that the issue I had initially was because we were only giving write access to kubeapps-operator, where I explicitly updated this in the PR so that the UI requires both read and write to use the add-app repo UI. I've added read access and it gets past there, but fails elsewhere. Landing your PR #1606 which I'll merge and retry...

@absoludity absoludity force-pushed the 1521-default-to-per-namespace-apprepos branch 4 times, most recently from 30ae336 to ca8151d Compare March 26, 2020 04:59
@absoludity absoludity force-pushed the 1521-default-to-per-namespace-apprepos branch from ca8151d to d7b7410 Compare March 26, 2020 05:43
@absoludity
Copy link
Contributor Author

@andresmgot I'll fix the conflict, but I'm not sure why this is failing on CI with the admin user not having access to fetch AppRepos in the kubeapps namespace? (see https://19293-107555654-gh.circle-artifacts.com/0/integration/reports/screenshots/creates-a-registry.png , which is from integration/use-cases/create-registry.js which logs in with ADMIN_TOKEN. I'll look more closely in the morning.

The error could be misleading, if the component is receiving a forbidden error and the error message is not parse-able as a Forbidden error it shows the default required RBAC roles error that is the one you are seeing. Another possibility is that the admin user has RBAC permissions to manage apprepositories only in the kubeapps namespace (ref, since that namespace is now like a "global" namespace that can be also the error if we are requesting apprepositories in all namespaces.

Thanks for the background. That helped me see that the issue I had initially was because we were only giving write access to kubeapps-operator, where I explicitly updated this in the PR so that the UI requires both read and write to use the add-app repo UI. I've added read access and it gets past there, but fails elsewhere. Landing your PR #1606 which I'll merge and retry...

So, frustratingly, this appears to be similar to something we've experienced in the past requiring a second click. In this case, the first click appears to only give the button focus, works fine with the second :/ See d7b7410 for the fix.

@absoludity absoludity merged commit c04046d into master Mar 26, 2020
@absoludity absoludity deleted the 1521-default-to-per-namespace-apprepos branch March 26, 2020 06:08
@andresmgot
Copy link
Contributor

So, frustratingly, this appears to be similar to something we've experienced in the past requiring a second click. In this case, the first click appears to only give the button focus, works fine with the second :/ See d7b7410 for the fix.

it's indeed weird to see this behavior from time to time.. I wonder if it's an issue in our side or the test framework behaving weird with modals.

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.

2 participants