Skip to content

Add IBM Power VS: tf data#5614

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
clnperez:new-tf-data
Apr 13, 2022
Merged

Add IBM Power VS: tf data#5614
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
clnperez:new-tf-data

Conversation

@clnperez
Copy link
Copy Markdown
Contributor

@clnperez clnperez commented Feb 4, 2022

For more background on IPI on Power VS, refer to the enhancement
proposal here: openshift/enhancements#736

Older discussions on some of the code here can be found in #5224

Signed-off-by: Christy Norman christy@linux.vnet.ibm.com

@openshift-ci openshift-ci Bot requested review from jhixson74 and rna-afk February 4, 2022 23:53
@clnperez
Copy link
Copy Markdown
Contributor Author

clnperez commented Mar 3, 2022

this needs some rework post #5507 and to stop relying on the custom s3 plugin for the bootstrap ignition config

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2022
@clnperez
Copy link
Copy Markdown
Contributor Author

i'm going to add a few more bits to this so we have fewer PRs to review, b/c of the timing with the tests mentioned in other Power VS PRs.

@clnperez clnperez changed the title Add IBM Power VS: tf data WIP Add IBM Power VS: tf data Mar 15, 2022
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2022
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2022
@clnperez clnperez changed the title WIP Add IBM Power VS: tf data Add IBM Power VS: tf data Mar 22, 2022
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 22, 2022
@clnperez
Copy link
Copy Markdown
Contributor Author

@patrickdillon @rna-afk -- I'm taking this back out of WIP and will push new changes to a new PR. The "small thing" I wanted to add here turned out to be a little more like a medium thing.

@clnperez clnperez force-pushed the new-tf-data branch 4 times, most recently from 57d898f to debe676 Compare March 25, 2022 20:21
@clnperez
Copy link
Copy Markdown
Contributor Author

@rna-afk @patrickdillon @Prashanth684 can you take a look at this as well? Depends on #5615

@clnperez
Copy link
Copy Markdown
Contributor Author

clnperez commented Apr 5, 2022

/retest-required

@clnperez
Copy link
Copy Markdown
Contributor Author

clnperez commented Apr 6, 2022

ping @rna-afk do you know if the list of required/optional tests only shows up after an lgtm?

@rna-afk
Copy link
Copy Markdown
Contributor

rna-afk commented Apr 6, 2022

@patrickdillon has been working on clearing up the CI tests that are blocking PRs from merging. I'm hoping that's the reason we aren't seeing any required tests right now.

Comment thread data/data/powervs/cluster/bootstrap/variables.tf Outdated
Comment thread data/data/powervs/cluster/dns/variables.tf Outdated
Comment thread data/data/powervs/cluster/master/variables.tf Outdated
Comment thread data/data/powervs/cluster/loadbalancer/variables.tf Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this commented because we need to test it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no those i probably should have deleted but they're removed in #5780

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rna-afk i'm going to resolve this an assume it's okay to leave as-is b/c it's taken care of in the other pr. lmk if that's not okay.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would prefer not to merge code to remove it in a later pr/commit & #5780 still needs to be broken into separate commits. Can we get rid of this and the other unnecessary comments?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure. deleted this whole file and re-pushed

@clnperez
Copy link
Copy Markdown
Contributor Author

clnperez commented Apr 7, 2022

@mjturek -- can you double-check the descriptions I just added (esp the COS ones for bootstrap)?

Comment thread data/data/powervs/cluster/bootstrap/main.tf Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is okay, but "The contents of the ignition file" may be more accurate.

Comment thread data/data/powervs/cluster/bootstrap/variables.tf Outdated
Copy link
Copy Markdown
Contributor

@mjturek mjturek left a comment

Choose a reason for hiding this comment

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

Comments inline

@clnperez clnperez force-pushed the new-tf-data branch 2 times, most recently from 37c29c5 to d83772e Compare April 7, 2022 18:09
@clnperez
Copy link
Copy Markdown
Contributor Author

clnperez commented Apr 7, 2022

thanks @mjturek!

should be all good now @rna-afk

@clnperez
Copy link
Copy Markdown
Contributor Author

clnperez commented Apr 8, 2022

/retest-required

@patrickdillon these two keep failing for this and #5615. do i need to keep retrying? if a PR gets an approve does mergebot automatically retry for me?

Copy link
Copy Markdown
Contributor

@patrickdillon patrickdillon left a comment

Choose a reason for hiding this comment

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

This looks very close. But I would prefer to remove the unneeded comments, especially considering one is an entire file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would prefer not to merge code to remove it in a later pr/commit & #5780 still needs to be broken into separate commits. Can we get rid of this and the other unnecessary comments?

For more background on IPI on Power VS, refer to the enhancement
proposal here: openshift/enhancements#736

Older discussions on some of the code here can be found in openshift#5224

Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>
@patrickdillon
Copy link
Copy Markdown
Contributor

/approve

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 11, 2022

@clnperez: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-single-node 9e4712f6bed53551b11e849dfb79cd26189b3eaa link false /test e2e-aws-single-node
ci/prow/e2e-aws-fips 9e4712f6bed53551b11e849dfb79cd26189b3eaa link false /test e2e-aws-fips
ci/prow/e2e-ibmcloud 9e4712f6bed53551b11e849dfb79cd26189b3eaa link false /test e2e-ibmcloud
ci/prow/e2e-azure-upi 9e4712f6bed53551b11e849dfb79cd26189b3eaa link false /test e2e-azure-upi
ci/prow/e2e-openstack 9e4712f6bed53551b11e849dfb79cd26189b3eaa link false /test e2e-openstack
ci/prow/e2e-libvirt 9e4712f6bed53551b11e849dfb79cd26189b3eaa link false /test e2e-libvirt
ci/prow/e2e-crc 9e4712f6bed53551b11e849dfb79cd26189b3eaa link false /test e2e-crc
ci/prow/e2e-aws-workers-rhel8 9e4712f6bed53551b11e849dfb79cd26189b3eaa link false /test e2e-aws-workers-rhel8
ci/prow/e2e-ovirt 9e4712f6bed53551b11e849dfb79cd26189b3eaa link false /test e2e-ovirt
ci/prow/okd-e2e-aws-upgrade debe6764f558345410e493d352568b00e47131b9 link false /test okd-e2e-aws-upgrade
ci/prow/e2e-gcp-upgrade debe6764f558345410e493d352568b00e47131b9 link true /test e2e-gcp-upgrade
ci/prow/e2e-aws-upgrade debe6764f558345410e493d352568b00e47131b9 link true /test e2e-aws-upgrade
ci/prow/e2e-openstack-parallel a87cf69 link false /test e2e-openstack-parallel
ci/prow/e2e-openstack-proxy a87cf69 link false /test e2e-openstack-proxy
ci/prow/e2e-aws-disruptive a87cf69 link false /test e2e-aws-disruptive
ci/prow/e2e-azure-resourcegroup a87cf69 link false /test e2e-azure-resourcegroup
ci/prow/e2e-aws-shared-vpc a87cf69 link false /test e2e-aws-shared-vpc
ci/prow/e2e-gcp-upi-xpn a87cf69 link false /test e2e-gcp-upi-xpn
ci/prow/e2e-gcp-shared-vpc a87cf69 link false /test e2e-gcp-shared-vpc
ci/prow/e2e-azure-shared-vpc a87cf69 link false /test e2e-azure-shared-vpc

Full PR test history. Your PR dashboard.

Details

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.

@clnperez
Copy link
Copy Markdown
Contributor Author

/retest-required

@clnperez
Copy link
Copy Markdown
Contributor Author

@patrickdillon can you skip those two for this pr as well? nothing seems to be re-running anyway

@rna-afk
Copy link
Copy Markdown
Contributor

rna-afk commented Apr 13, 2022

/lgtm
/skip

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2022
@openshift-merge-robot openshift-merge-robot merged commit 5762e57 into openshift:master Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants