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.

Sign up for GitHub

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

Jump to bottom

upi: Post TF 0.12.x migration fixups #3494

Merged
merged 2 commits into from Jun 5, 2020
Merged

upi: Post TF 0.12.x migration fixups #3494

merged 2 commits into from Jun 5, 2020

Conversation

LorbusChris
Copy link
Member

@LorbusChris LorbusChris commented Apr 22, 2020 •
edited

In this PR:

  • upi/vsphere/lb: Use decimal file mode for haproxy.conf
    To align with the other files written by the Terraform Ignition provider.
    This also changes the file mode to 0644 as it does not need the
    executable bit.
  • upi/vsphere: Update README.md and example tfvars
    This reflects recent changes
  • Dockerfile.upi.ci: Update govmomi to v0.22.2
    This aligns it with the version currently vendored via go.mod     this release doesn't provide all the binaries

@LorbusChris
Copy link
Member Author

/test e2e-vsphere-upi

@LorbusChris LorbusChris changed the title upi/vsphere/lb: Fix haproxy file mode upi/vsphere/lb/main.tf: Fix haproxy.conf file mode Apr 22, 2020
@LorbusChris
Copy link
Member Author

/test e2e-vsphere-upi

@jcpowermac
Copy link
Contributor

@LorbusChris
https://www.terraform.io/docs/providers/ignition/d/file.html#mode
did you see an issue because of the octal vs decimal?

@LorbusChris
Copy link
Member Author

LorbusChris commented Apr 22, 2020 •
edited

I did not, and I haven't checked whether it might actually supported in the tf ignition provider. This however aligns it with the other files.

Additional Note: octal mode is NOT supported in pure Ignition json configs, so I've definitely run into this issue before in other contexts

@LorbusChris
Copy link
Member Author

the default is 0644, in case this were invalid. does this file have to be executable anyway?

@LorbusChris
Copy link
Member Author

LorbusChris commented Apr 22, 2020 •
edited

sorry, on the phone, only just now saw the docs. looks like it is supported by the provider after all :) I'll close if you prefer

@jcpowermac
Copy link
Contributor

the default is 0644, in case this were invalid. does this file have to be executable anyway?

you are right it doesn't need to be executable. So its up to you if you want to still fix it or close.

@LorbusChris
Copy link
Member Author

/test e2e-vsphere-upi

@LorbusChris
Copy link
Member Author

@jcpowermac I added another commit to reflect some of the recent changes in the README and example tfvars. There's also docs/user/vsphere/install_upi.md which to me looks like it would need a bigger overhaul.

As this doesn't have a BZ I don't expect expect this to get merged before master/4.6 opens.

@LorbusChris
Copy link
Member Author

/test e2e-vsphere-upi

@LorbusChris LorbusChris changed the title upi/vsphere/lb/main.tf: Fix haproxy.conf file mode upi: Post TF 0.12.x migration fixups Apr 22, 2020
@LorbusChris
Copy link
Member Author

/test e2e-vsphere-upi

@jcpowermac
Copy link
Contributor

@LorbusChris thanks for doing this!

@jcpowermac
Copy link
Contributor

vSphere failed from removed commit retry and is running into vSphere CI env limits
/test e2e-vsphere-upi

@jcpowermac
Copy link
Contributor

I am not sure what is up with this. I can download the file and extract perfectly fine.

could not run steps: step upi-installer failed: could not wait for build: the build upi-installer failed after 7m50s with reason DockerBuildFailed: Docker build strategy has failed.

Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --...9    0     0     30      0 --:--:-- --:--:-- --:--:--    30

gzip: govc_linux_amd64.gz: not in gzip format
error: build error: running 'curl -L -O https://github.com...4 && mv govc_linux_amd64 /bin/govc' failed with exit code 1

@LorbusChris
Copy link
Member Author

I've seen that a few times before, so far it's always been a transient infra issue

@LorbusChris
Copy link
Member Author

/retest

1 similar comment
@LorbusChris
Copy link
Member Author

/retest

@LorbusChris
Copy link
Member Author

prom didn't come up
/test e2e-vsphere-upi

@LorbusChris
Copy link
Member Author

/test e2e-vsphere-upi

@LorbusChris
upi/vsphere/lb: Use decimal file mode for haproxy.conf
da732ab 
To align with the other files written by the Terraform Ignition provider.
This also changes the file mode to 0644 as it does not need the
executable bit.
@LorbusChris
upi/vsphere: Update README, docs and example tfvars
10f82ed 
This reflects recent changes.
@LorbusChris
Copy link
Member Author

/test e2e-vsphere-upi
/cc @jcpowermac

Now that 4.6 is open, do you mind having another look at this?

@jcpowermac
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2020
@LorbusChris
Copy link
Member Author

/retest

2 similar comments
@LorbusChris
Copy link
Member Author

/retest

@LorbusChris
Copy link
Member Author

/retest

@abhinavdahiya
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

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

The pull request process is described 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2020
@LorbusChris
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link
Contributor

@LorbusChris: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-ovirt 10f82ed link /test e2e-ovirt

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 3d59b56 into openshift:master Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabianofranz fabianofranz Awaiting requested review from fabianofranz

@mtnbikenc mtnbikenc Awaiting requested review from mtnbikenc

@jcpowermac jcpowermac Awaiting requested review from jcpowermac

Assignees

@jcpowermac jcpowermac

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@LorbusChris @jcpowermac @abhinavdahiya @openshift-ci-robot @openshift-bot @openshift-merge-robot