-
Notifications
You must be signed in to change notification settings - Fork 244
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 devfile registry on preferences init when ODO_EXPERIMENTAL=true #3845
Add devfile registry on preferences init when ODO_EXPERIMENTAL=true #3845
Conversation
Signed-off-by: John Collier <John.J.Collier@ibm.com>
Signed-off-by: John Collier <John.J.Collier@ibm.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: John Collier <John.J.Collier@ibm.com>
Signed-off-by: John Collier <John.J.Collier@ibm.com>
Infra flakes
/retest |
/retest |
Codecov Report
@@ Coverage Diff @@
## master #3845 +/- ##
==========================================
- Coverage 44.19% 44.17% -0.02%
==========================================
Files 141 141
Lines 13600 13608 +8
==========================================
+ Hits 6010 6011 +1
- Misses 7007 7013 +6
- Partials 583 584 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnmcollier Thanks for the fix, please fix here too to avoid panic in future, dereferencing a pointer without checking for nil
https://github.com/openshift/odo/blob/master/pkg/odo/cli/registry/list.go#L60
Signed-off-by: John Collier <John.J.Collier@ibm.com>
@adisky Thanks! Good catch. I've updated this PR to include the nil check 👍 |
// Add the default devfile registry if "ODO_EXPERIMENTAL" env variable is true | ||
// We specifically need to check for the experimental mode env, as `odo preferences set experimental true` already properly | ||
// initializes the default devfile registries. | ||
experimentalEnvStr, _ := os.LookupEnv("ODO_EXPERIMENTAL") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this check? Should we even bother checking if we're in experimental mode? I have applied your patch to my PR here: #3705 which doesn't include looking up experimental and I have all tests passing.
Just out of curiosity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cdrage This test https://github.com/openshift/odo/blob/master/tests/integration/component.go#L293 was failing without this check, as devfile components were getting listed without experimental mode being on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like your branch modifies the test to remove that check (which makes sense, as devfile is being moved out of experimental mode), but that explains why the test isn't failing on your branch
@johnmcollier: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Closing this PR in favour of #3705 which has the same changes |
@johnmcollier charlie's PR does not have this change https://github.com/openshift/odo/pull/3845/files#diff-0cd3824178637db7a863b63f797b00d4, you could reopen and update this PR OR include this changes in #3705 |
What type of PR is this?
/kind bug
What does does this PR do / why we need it:
This PR is mostly a backport of johnmcollier@274acb7, which was delivered into the odov2 alpha release
This PR fixes a panic that can be seen when
odo registry list
is called but the preferences file does not yet exist (and experimental mode is set viaODO_EXPERIMENTAL=true
). What was happening was that when retrieve a preferences file, we only add the default devfile registry if the preferences file already exists, if we have to create and initialize the preferences file, we don't add the registry. So a simple solution is to make sure we add the devfile registries when we create and initialize the preferences file.I also added an integration test case to verify this scenario. As this bug was specific to when
ODO_EXPERIMENTAL
was set, I had to add some wrapper functions that allow us to pass in env variables to commands in the test framework.Which issue(s) this PR fixes:
Fixes #3842
PR acceptance criteria:
Unit test
Integration test
Documentation
I have read the test guidelines
How to test changes / Special notes to the reviewer:
~/.odo/preference.yaml
export ODO_EXPERIMENTAL=true
odo registry list
, verify the default registry is listed and noo panic occurs.