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

Add explicit scope to the new repo form #5240

Merged
merged 1 commit into from Aug 30, 2022

Conversation

antgamdia
Copy link
Contributor

@antgamdia antgamdia commented Aug 29, 2022

Description of the change

Adding an explicit scope for selecting whether the repo should be namespace or global + some info messages to the user.

Benefits

Adding a global repo will no longer require switching the context, which wasn't something trivial to figure out as a user.

Possible drawbacks

N/A

Applicable issues

Additional information

reposWithScope

Squeezing two changes:

  • Disable the description field if using carvel repos (as it currently errors if any is passed)
  • Fix some tags to comply with a11y recommendations.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@netlify
Copy link

netlify bot commented Aug 29, 2022

Deploy Preview for kubeapps-dev canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 163d479
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/630cf72a2b75ea0008239afb

@ppbaena
Copy link
Collaborator

ppbaena commented Aug 30, 2022

Nice to see this UX improvement when configuring repositories. Thanks @antgamdia!

Comment on lines +362 to +365
namespace: isNamespaceScoped
? namespace
: getGlobalNamespaceOrNamespace(namespace, plugin?.name, currentNsConfig),
isNamespaceScoped,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing either the current namespace or the corresponding global one if "global" is selected.

Comment on lines +469 to +470
setIsNamespaceScoped(
!isGlobalNamespace(namespace, PluginNames.PACKAGES_FLUX, currentNsConfig),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering if we should change the setIsNamespaceScoped with setIsGlobalScoped to match the isGlobal function. However, the parameter used in the API is namespaceScoped, so I've tried to stick to this API field name.
Happy to change if you think otherwise

@@ -543,12 +579,15 @@ export function PkgRepoForm(props: IPkgRepoFormProps) {
const userManagedSecretText = "Use an existing secret";
const kubeappsManagedSecretText = "Provide the secret values";

const isUserManagedSecretToggle = (
const isUserManagedSecretToggle = (section: string) => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just adding the proper id and for tags to the labels to comply with the a11y requirements. The linter missed it bc it is being dynamically generated.

@@ -637,6 +676,7 @@ export function PkgRepoForm(props: IPkgRepoFormProps) {
placeholder="Description of the repository"
value={description || ""}
onChange={handleDescriptionChange}
disabled={plugin?.name === (PluginNames.PACKAGES_KAPP as string)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disabling description if the plugin is kapp

Comment on lines +745 to +746
{repo.namespaceScoped ||
(isGlobalNamespace(namespace, plugin?.name, currentNsConfig) && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using either the field from the repo object (editing the existing repo) or calculating if the ns is global or not.

@@ -235,3 +235,21 @@ export function isGlobalNamespace(namespace: string, pluginName: string, kubeapp
return false;
}
}

export function getGlobalNamespaceOrNamespace(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should belong to each plugin's config

Copy link
Collaborator

@castelblanque castelblanque left a comment

Choose a reason for hiding this comment

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

LGTM

@antgamdia antgamdia merged commit 6fe621b into vmware-tanzu:main Aug 30, 2022
@antgamdia antgamdia deleted the 2995-explicitScopeRepos branch August 30, 2022 11:57
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.

No obvious way to create global app repository
4 participants