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

add ocp install log, delete cluster #10703

Conversation

DanielOsypenko
Copy link
Contributor

  1. tuned create_openshift_install_log_file and added to rosahcp
  2. added delete resources such as oidc-config, account-roles, etc into destroy_cluster

@DanielOsypenko DanielOsypenko added the platforms Related to support of deployment or test execution on particular platforms (baremetal, AWS, ...) label Oct 20, 2024
@DanielOsypenko DanielOsypenko self-assigned this Oct 20, 2024
@DanielOsypenko DanielOsypenko requested a review from a team as a code owner October 20, 2024 15:46
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines label Oct 20, 2024
@DanielOsypenko DanielOsypenko force-pushed the rosa_hcp_deploy_and_destroy_leftovers branch from 5388585 to 9512c39 Compare October 20, 2024 16:26
Signed-off-by: Daniel Osypenko <dosypenk@redhat.com>
@DanielOsypenko DanielOsypenko force-pushed the rosa_hcp_deploy_and_destroy_leftovers branch from 9512c39 to 2346f9f Compare October 21, 2024 06:41
Signed-off-by: Daniel Osypenko <dosypenk@redhat.com>
Signed-off-by: Daniel Osypenko <dosypenk@redhat.com>
@DanielOsypenko DanielOsypenko force-pushed the rosa_hcp_deploy_and_destroy_leftovers branch 2 times, most recently from 60a0f39 to ff96098 Compare October 21, 2024 16:15
Signed-off-by: Daniel Osypenko <dosypenk@redhat.com>
@DanielOsypenko DanielOsypenko force-pushed the rosa_hcp_deploy_and_destroy_leftovers branch from ff96098 to b86e840 Compare October 21, 2024 16:18
@DanielOsypenko
Copy link
Contributor Author

@DanielOsypenko DanielOsypenko added the Verified Mark when PR was verified and log provided label Oct 22, 2024
jilju
jilju previously approved these changes Oct 22, 2024
dahorak
dahorak previously approved these changes Oct 22, 2024
Copy link
Contributor

@dahorak dahorak left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I would just propose to update the message in the .openshift_install.log to make it more generic also for other types of deployment, not only via Assisted Installer.

fd.writelines(
[
"W/A for our CI to get URL to the cluster in jenkins job. "
"Cluster is deployed via Assisted Installer API!\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth to update this line based on the deployment type (or make it more generic), as this is not true for all the cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not change this line, I only pushed create_openshift_install_log_file function to common space from ocs_ci/deployment/assisted_installer.py
as far as I understand it is a mock-up for our log-parser on Jenkins pipelines.

TBH, correcting code(mocked-up log message) for our custom-built pipelines that are not validating this log message other than retrieving the console should have a very low priority. Until it can be important for something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the Jenkins job, the important line is only the last one ... Access the OpenShift web-console here ....

The think is, that initially it was used only for the Assisted Installer deployment, where it make sense the message, but since you are moving it to the common space and re-using it for the ROSA deployment also, it will be worth to update the message to avoid confusion.
I think it is not necessary to make it dynamic based on the actual deployment type, but just more generic. What about something like this?

"Cluster is deployed via some kind of managed deployment (Assisted Installer API or ROSA). OpenShift Installer (IPI or UPI deployment) were not used!\n"

Signed-off-by: Daniel Osypenko <dosypenk@redhat.com>
@DanielOsypenko DanielOsypenko dismissed stale reviews from dahorak and jilju via 7872cb8 October 23, 2024 09:05
@openshift-ci openshift-ci bot removed the lgtm label Oct 23, 2024
@openshift-ci openshift-ci bot added the lgtm label Oct 23, 2024
Copy link

openshift-ci bot commented Oct 23, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dahorak, DanielOsypenko, jilju

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dahorak dahorak merged commit 8e4e731 into red-hat-storage:master Oct 23, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm platforms Related to support of deployment or test execution on particular platforms (baremetal, AWS, ...) size/L PR that changes 100-499 lines Verified Mark when PR was verified and log provided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants