-
Notifications
You must be signed in to change notification settings - Fork 208
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
OCM-9780 | test: automated id:65900 install with use-local-credentials will work #2396
base: master
Are you sure you want to change the base?
Conversation
test LGTM, could you explain to me what the flag does just for my own sake? |
@OAharoni-RedHat I believe it just makes it so that when you install a non-sts cluster it will use your AWS credentials from your machine |
@jerichokeyne maybe i'm going to out myself as clueless here but isn't that what it already does? Since we need to set up an |
I'll be honest I'm not completely sure what the difference is. I think the difference is that both will use your local credentials for installing, but this flag makes the cluster use your credentials instead of the osdCcsAdmin credentials. It's apparently something we don't fully support even though it's something we test for 🤷 |
4b586f8
to
15157ec
Compare
It looks like the step for testing vs an STS cluster is missing. It might make sense to split that into a separate negative test case. Also, does the cluster itself describe that it used local credentials in any way? Or is it only evident in the json structure? |
Ah, looks like I forgot that part. I'll have to do some more research into how to do that since it's an error at install time
As far as I know it's only evident in the json |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2396 +/- ##
==========================================
+ Coverage 29.86% 29.89% +0.03%
==========================================
Files 171 171
Lines 23541 23316 -225
==========================================
- Hits 7031 6971 -60
+ Misses 15939 15776 -163
+ Partials 571 569 -2 ☔ View full report in Codecov by Sentry. |
15157ec
to
7d73f4c
Compare
@jfrazierRH I added that negative test case |
/lgtm |
7d73f4c
to
6b01ca4
Compare
New changes are detected. LGTM label has been removed. |
6b01ca4
to
9ac69f5
Compare
/retest |
admin_enabled: false | ||
- as: rosa-non-sts-local-creds |
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.
Is that meant to be launched from a job ?
If yes, I don't think we need a new profile. You could have here this profile property be configurable also via an environment variable USE_LOCAL_CREDENTIALS
which would set the profile property accordingly and configure your job to use the rosa-non-sts-advanced
profile with the env var USE_LOCAL_CREDENTIALS: true
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.
I added a env var option now. I set it up currently so that when set to true it will enable the profile option, but if it's set to anything else it'll leave the profile option alone
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.
so you don't need to keep that profile here. And you would need to create a new job in openshift/release
for this specific case
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jerichokeyne, jfrazierRH 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 |
1d6faab
to
94fa747
Compare
94fa747
to
7d55eaf
Compare
…redentials will work
7d55eaf
to
5bc9ead
Compare
/retest |
@jerichokeyne: 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-sigs/prow repository. I understand the commands that are listed here. |
config.Test.GlobalENV.ComputeMachineType) | ||
profile.ClusterConfig.InstanceType = config.Test.GlobalENV.ComputeMachineType | ||
} | ||
|
||
if config.Test.GlobalENV.UseLocalCredentials { | ||
log.Logger.Info("Got global env setting for USE_LOCAL_CREDENTIALS, overwritten the profile setting to true") | ||
profile.ClusterConfig.UseLocalCredentials = true |
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.
please check that config.Test.GlobalENV.UseLocalCredentials == "true"
PR needs rebase. 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-sigs/prow repository. |
Automated id:65900 which tests that a cluster installed with the
--use-local-credentials
flag will install correctlyoutput: