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

Clarify the description of "namespace" in the UI and CLI #1023

Merged
merged 8 commits into from Nov 8, 2021

Conversation

joshri
Copy link
Contributor

@joshri joshri commented Nov 3, 2021

Closes #898

Changes the description of the namespace field in the UI (ApplicationAdd), as well as the --namespace flag in gitops/root + README.

@joshri joshri added the area/ui Issues that require front-end work label Nov 3, 2021
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

Check out my suggestion and if you like it, let's use it everywhere. The fact that this text is repeated three times highlights a need for some sort of application text catalog.

@@ -49,7 +49,7 @@ Available Commands:

Flags:
-h, --help Help for gitops
--namespace string Weave GitOps runtime namespace (default "wego-system")
--namespace string The namespace scope for this operation (default "wego-system").
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a change I did in a branch. Trying to be super specific about what this flag does.

Suggested change
--namespace string The namespace scope for this operation (default "wego-system").
--namespace string The Kubernetes namespace in which Weave GitOps objects will be stored (default "wego-system").

Copy link
Contributor

Choose a reason for hiding this comment

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

@jpellizzari specific can be tricky... doing a get/status or remove isn't a "will be", it's "the objects are here already"

Copy link
Contributor

Choose a reason for hiding this comment

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

@sympatheticmoose Not sure what you mean. We are open to suggestions on the text itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I gave my suggestion already in the issue ;) I'm saying "will be stored" implies that this is always an action where I am about to store objects which don't yet exist into the given namespace. Which isn't the case for read or remove operations.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I missed that. @joshri ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it 🌞

@jpellizzari jpellizzari marked this pull request as ready for review November 4, 2021 17:33
Copy link
Contributor

@J-Thompson12 J-Thompson12 left a comment

Choose a reason for hiding this comment

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

LGTM

@joshri joshri merged commit 69275f6 into main Nov 8, 2021
@joshri joshri deleted the 898-namespace-text branch November 8, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui Issues that require front-end work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clearer guidance on namespace field in the UI for adding applications and --namespace flag in the CLI
4 participants