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

update ods-ci to use the new hive server #1307

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

kobihk
Copy link
Contributor

@kobihk kobihk commented Mar 18, 2024

related to jira ticket: RHOAIENG-3338

Signed-off-by: Kobi Hakimi [email protected]

@kobihk kobihk self-assigned this Mar 18, 2024
@@ -145,6 +146,7 @@

Wait For Cluster To Be Ready
${pool_namespace} = Get Cluster Pool Namespace ${pool_name}
Set Task Variable ${pool_namespace}

Check notice

Code scanning / Robocop

{{ set_variable_keyword }} can be replaced with VAR Note

Set Task Variable can be replaced with VAR
@@ -145,6 +146,7 @@

Wait For Cluster To Be Ready
${pool_namespace} = Get Cluster Pool Namespace ${pool_name}
Set Task Variable ${pool_namespace}

Check warning

Code scanning / Robocop

Test, suite and global variables should be uppercase Warning

Test, suite and global variables should be uppercase
Copy link
Contributor

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass %
448 0 0 448 100

Copy link
Member

@jstourac jstourac left a comment

Choose a reason for hiding this comment

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

Basically LGTM, one question; two proposals.

Also, it would be great not to mix the linter changes into the very same commit with the actual changes. It's harder to review it then.

@kobihk kobihk force-pushed the support_new_hive_master branch from 7dc2671 to fe822ff Compare March 19, 2024 08:51
@kobihk
Copy link
Contributor Author

kobihk commented Mar 19, 2024

Basically LGTM, one question; two proposals.

Also, it would be great not to mix the linter changes into the very same commit with the actual changes. It's harder to review it then.

The changes that remove redundant spaces and add new lines were fixed automatically by my IDE and I think it better to fix it from time to time if we can.
About the rest of the comments - fixed!!

@kobihk kobihk requested a review from jstourac March 19, 2024 08:58
aloganat
aloganat previously approved these changes Mar 19, 2024
@jstourac
Copy link
Member

The changes that remove redundant spaces and add new lines were fixed automatically by my IDE and I think it better to fix it from time to time if we can. About the rest of the comments - fixed!!

I agree but even in this case you can put these changes into a separate commits. Should be easy with the git add -p ....

jstourac
jstourac previously approved these changes Mar 19, 2024
Comment on lines +11 to +12
export AWS_ACCESS_KEY_ID=${6:-$AWS_ACCESS_KEY_ID}
export AWS_SECRET_ACCESS_KEY=${7:-$AWS_SECRET_ACCESS_KEY}
Copy link
Contributor

@manosnoam manosnoam Mar 19, 2024

Choose a reason for hiding this comment

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

Why do you need to pass credentials into the script args ? It is not too secured.
Can you set the AWS access before calling the script ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we use the AWS hive credentials all over the provision process
here inside the script, the export limited the effect of changing of the AWS keys to this script only without the necessary of switch between profiles

Copy link
Contributor

Choose a reason for hiding this comment

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

But these credentials are not used within the create_fips.sh:

https://github.com/red-hat-data-services/ods-ci/blob/fe822ffd45d054d580d12c019fa03bc1fc31ff03/ods_ci/tasks/Resources/Provisioning/Hive/OSP/create_fips.sh#L11C1-L13C1

I think you can remove them, and just switch AWS profile as needed before calling the script:

aws configure set profile $AWS_PROFILE
aws configure set aws_access_key_id ... --profile ${AWS_PROFILE}
aws configure set aws_secret_access_key ... --profile ${AWS_PROFILE}
aws configure --profile "$AWS_PROFILE"

But If you must use them inside the script as plain text (unsecured), please call it inside a subshell to hide output.

Copy link
Contributor

@manosnoam manosnoam Mar 19, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@kobihk kobihk Mar 19, 2024

Choose a reason for hiding this comment

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

as you can see from the console the key and secret are masked

10:50:42  Creating Openstack Floating IPs and AWS DNS Records
10:50:42  Watching command output: /home/jenkins/workspace/rhods/rhods-smoke/ods-ci/ods_ci/tasks/Resources/Provisioning/Hive/OSP/create_fips.sh ods-qe-psi-02 [osp.rh-ods.com](http://osp.rh-ods.com/) provider_net_cci_14 rhos-d /home/jenkins/workspace/rhods/rhods-smoke/ods-ci/ods_ci/ods_ci/test-output/ods-ci-2024-03-19-08-50-CIdpRO2oYu/ **** ****

@kobihk kobihk added the do not merge Do not merge this yet please label Mar 19, 2024
@kobihk kobihk requested a review from manosnoam March 19, 2024 09:49
Comment on lines +11 to +12
export AWS_ACCESS_KEY_ID=${6:-$AWS_ACCESS_KEY_ID}
export AWS_SECRET_ACCESS_KEY=${7:-$AWS_SECRET_ACCESS_KEY}
Copy link
Contributor

Choose a reason for hiding this comment

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

But these credentials are not used within the create_fips.sh:

https://github.com/red-hat-data-services/ods-ci/blob/fe822ffd45d054d580d12c019fa03bc1fc31ff03/ods_ci/tasks/Resources/Provisioning/Hive/OSP/create_fips.sh#L11C1-L13C1

I think you can remove them, and just switch AWS profile as needed before calling the script:

aws configure set profile $AWS_PROFILE
aws configure set aws_access_key_id ... --profile ${AWS_PROFILE}
aws configure set aws_secret_access_key ... --profile ${AWS_PROFILE}
aws configure --profile "$AWS_PROFILE"

But If you must use them inside the script as plain text (unsecured), please call it inside a subshell to hide output.

@kobihk kobihk dismissed stale reviews from jstourac and aloganat via 701ca3a March 19, 2024 11:37
@kobihk kobihk force-pushed the support_new_hive_master branch from fe822ff to 701ca3a Compare March 19, 2024 11:37
${result} = Run Process
... oc -n ${pool_namespace} get cd ${pool_namespace} -o json | jq -r '.status.webConsoleURL' --exit-status
... shell=yes
Should Be True ${result.rc} == 0

Check notice

Code scanning / Robocop

'{{ block_name }}' condition can be simplified Note

'Should Be True' condition can be simplified
manosnoam
manosnoam previously approved these changes Mar 19, 2024
related to jira ticket: RHOAIENG-3338

Signed-off-by: Kobi Hakimi <[email protected]>
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@kobihk kobihk requested review from manosnoam and jstourac March 19, 2024 14:33
@kobihk kobihk requested a review from aloganat March 19, 2024 14:33
@kobihk kobihk merged commit 1d3cfaf into red-hat-data-services:master Mar 20, 2024
12 checks passed
@kobihk kobihk deleted the support_new_hive_master branch March 20, 2024 11:58
@kobihk kobihk added verified This PR has been tested with Jenkins and removed do not merge Do not merge this yet please labels Mar 20, 2024
jstourac added a commit that referenced this pull request Mar 28, 2024
This PR contains following fixes from the `master` branch:

* edafbcc (#1296)
* 5e7f4b1 (#1302)
* b6fb7f7 (#1307)
* 9485246 (#1314)
* abfb8a5 (#1316)
* fc76761 (#1317)
* 439e2b3 (#1306)
* 5db9e26 (#1324)

---

I kept the order in which these commits were merged in the `master`
branch and provided both link to the particular commit and also link to
the particular PR so it's clearer to understand later on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
verified This PR has been tested with Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants