List namespace right after namespace has been created #6922
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🔨 Deploy Preview deleted from internal cluster!
|
feloy
suggested changes
Jun 23, 2023
| if ns.Name == nco.namespaceName { | ||
| break L | ||
| } | ||
| } |
There was a problem hiding this comment.
I would add a sleep for let's say 50ms at this point, to not respawn the List operation too fast
openshift-ci
bot
added
the
lgtm
rm3l
reviewed
Jun 23, 2023
| @@ -105,7 +108,29 @@ func (nco *NamespaceCreateOptions) Run(ctx context.Context) (err error) { | |||
| if err != nil { | |||
| return err | |||
| } | |||
|
|
|||
| if nco.waitFlag { | |||
| var timeOut = time.After(nco.clientset.PreferenceClient.GetTimeout()) | |||
There was a problem hiding this comment.
The default value for nco.clientset.PreferenceClient.GetTimeout() is currently 1 second. Isn't that too low?
There was a problem hiding this comment.
Also, to me, if the user sets --wait, it means that they purposely are willing to wait until they get confirmation that the namespace/project is returned correctly.
I'm thinking for example of a script using odo create namespace --wait before doing other things..
The user would need to interrupt the process to cancel the wait.
So IMO, we should not even have a timeout.
There was a problem hiding this comment.
For me, it's a matter of 10's of milliseconds. Having to break manually can be dangerous if executed from a script: is the user expecting the command will never return in case of problem? I would instead return an error if the namespace is not created after a reasonable time. As the timeout is configurable, I would keep as is, and users in heavy loaded systems could increase it.
There was a problem hiding this comment.
Yup, that makes sense. Personally, I found this Timeout preference confusing and not used consistently. I think every command with a --wait flag should also provide an additional --timeout flag to set a timeout only for this command, but that's a different story.
Alright, in this case, returning an error if the timeout is reached would indeed make more sense. The error should also clearly indicate how the user can adjust this timeout, I think.
cc @valaparthvi
rm3l
reviewed
Jun 23, 2023
There was a problem hiding this comment.
One last comment: I think the success message (Namespace "$ns" is ready for use) should be displayed only after the new checks are done. Otherwise, the output might be confusing if the namespace still does not appear in the list after the timeout is reached, e.g.:
$ odo create namespace test-ns11 --wait
✓ Waiting for namespace to come up [11ms]
✓ Namespace "test-ns11" is ready for use
⚠ Timeout while waiting for namespace "test-ns11"; it may take a while to appear
I'd suggest displaying ✓ Namespace "test-ns11" is ready for use only if the namespace does appear in the list. What do you think?
EDIT: Actually, I think the Waiting for namespace to come up line should return successfully only after we are done with the checks. Then we could display ✓ Namespace "$ns" is ready for use.
rm3l
added
the
do-not-merge/hold
openshift-ci
bot
removed
the
lgtm
rm3l
requested changes
Jun 27, 2023
rm3l
removed
the
do-not-merge/hold
rm3l
approved these changes
Jun 27, 2023
openshift-ci
bot
added
the
lgtm
rm3l
reviewed
Jun 27, 2023
rm3l
added
the
do-not-merge/hold
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
valaparthvi
force-pushed
the
6827-w-flag-for-odo-create-namespace-doesn-t-work-as-expected-on-crc-2.18-and-2.19
branch
from
4cbc111
to
b3d20bf
Compare
openshift-ci
bot
removed
the
lgtm
Signed-off-by: Parthvi Vala <pvala@redhat.com>
valaparthvi
force-pushed
the
6827-w-flag-for-odo-create-namespace-doesn-t-work-as-expected-on-crc-2.18-and-2.19
branch
from
b3d20bf
to
14b846b
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
rm3l
approved these changes
Jun 27, 2023
openshift-ci
bot
added
the
lgtm
rm3l
removed
the
do-not-merge/hold
Unrelated, but keeping an eye on this to see if it occurs again. /override windows-integration-test/Windows-test |
|
@rm3l: Overrode contexts on behalf of rm3l: windows-integration-test/Windows-test In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-merge-robot
merged commit 3bf5ffc
into
redhat-developer:main
23 checks passed
openshift-merge-robot
temporarily deployed
to
internal
GitHub Actions
Inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this:
/kind bug
/flake
What does this PR do / why we need it:
This PR fixes the bug where
odo create namespace -wdoes not work as expected.Which issue(s) this PR fixes:
Fixes #6827
PR acceptance criteria:
Unit test
Integration test
Documentation
How to test changes / Special notes to the reviewer: