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

[OCPCLOUD-1162] Implement render binary for use in installer #45

Merged
merged 8 commits into from
Jun 4, 2021
Merged

[OCPCLOUD-1162] Implement render binary for use in installer #45

merged 8 commits into from
Jun 4, 2021

Conversation

Danil-Grigorev
Copy link
Contributor

@Danil-Grigorev Danil-Grigorev commented May 18, 2021 •
edited

For review: https://github.com/openshift/cluster-cloud-controller-manager-operator/pull/45/files/7ac280f0c47a919d6a0b4ef914bde93ec671889f..0ee19ba06b9d7914f1fa74d96eb9c5bfc9d6eb32

Based on #42
Used in openshift/installer#4947

Starts Cluster Cloud Controller Manager in render mode

Usage:
  run [flags]

Flags:
      --cluster-infrastructure-file string   Input path for the cluster infrastructure file.
      --dest-dir string                      The destination dir where CCCMO writes the generated static pods for CCM.
  -h, --help                                 help for run
      --images-file string                   Input path for the images config map file.

To run locally:

make build
./bin/render run --images-file=manifests/0000_26_cloud-controller-manager-operator_01_images.configmap.yaml --dest-dir=cccmo-render --cluster-infrastructure-file=infra.yaml

@openshift-ci openshift-ci bot requested review from elmiko and Fedosin May 18, 2021 11:42
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2021
@Danil-Grigorev Danil-Grigorev force-pushed the implement-render branch 2 times, most recently from 31bf2ae to d1b990a Compare May 20, 2021 12:28
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2021
@Danil-Grigorev Danil-Grigorev mentioned this pull request May 21, 2021
Merged
@mandre mandre mentioned this pull request May 30, 2021
Closed
@Danil-Grigorev Danil-Grigorev force-pushed the implement-render branch 2 times, most recently from 7dec100 to f485600 Compare June 3, 2021 13:38
@lobziik lobziik mentioned this pull request Jun 3, 2021
Merged
elmiko
elmiko reviewed Jun 3, 2021
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

this generally makes sense to me, i tried it out locally and everything worked as expected. i have one small suggestion and i agree with @lobziik about adding a test on the output file contents.

cmd/render/main.go Show resolved Hide resolved
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2021
@Danil-Grigorev
Write rendered manifests to destination bootstrap dir
c161640
@Danil-Grigorev
Ensure bootstrap dir is always present
d7ddbf6 
Every bootstrap resource is placed and named under following pattern:

<destinationDir>/<bootstrapPrefix>/<resourceName>-<resourceKind>.yaml
@Danil-Grigorev
Revendor with cobra
1e0cc60
lobziik
lobziik reviewed Jun 4, 2021
View reviewed changes
pkg/render/render_test.go
filename := fmt.Sprintf("%s-%s.yaml", res.GetName(), strings.ToLower(res.GetObjectKind().GroupVersionKind().Kind))
found := assert.Contains(t, names, filename)

if found {
Copy link
Contributor

@lobziik lobziik Jun 4, 2021 •
edited

Choose a reason for hiding this comment

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

nit: I think it might make sense to split this case into two. This if condition block looks bit strange to me.
Up to you.

@lobziik
Copy link
Contributor

lobziik commented Jun 4, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 4, 2021

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-ccm 1e0cc60 link /test e2e-aws-ccm

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/test-infra repository. I understand the commands that are listed here.

Fedosin
Fedosin approved these changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@Fedosin Fedosin left a comment

Choose a reason for hiding this comment

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

It works well for me, so I don't want to hold it. If there are any issues we can fix it later.

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Fedosin

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2021
@openshift-merge-robot openshift-merge-robot merged commit a60cae2 into openshift:master Jun 4, 2021
@Danil-Grigorev Danil-Grigorev changed the title Implement render binary for use in installer [OCPCLOUD-1162] Implement render binary for use in installer Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lobziik lobziik lobziik left review comments

@Fedosin Fedosin Fedosin approved these changes

@elmiko elmiko Awaiting requested review from elmiko

Assignees

@lobziik lobziik

@Fedosin Fedosin

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
5 participants
@Danil-Grigorev @lobziik @elmiko @Fedosin @openshift-merge-robot