Skip to content

Conversation

@csrwng
Copy link
Contributor

@csrwng csrwng commented Nov 19, 2020

Part 2 of cluster profiles support for IBM Cloud.
Introduces a directory that will contain profile-specific patches.
Profile-specific manifests are generated with 'make profile-manifests'
and 'hack/verify-profile-manifests.sh' (called by 'make verify') verifies
that the manifests directory is up to date.

On IBM Cloud the Image Registry operator needs to run on worker nodes, thus the
master node selector needs to be removed from the operator deployment
manifest.

@csrwng
Copy link
Contributor Author

csrwng commented Nov 19, 2020

/hold
This should only merge when profiles support has been turned on in the CVO

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2020
@openshift-merge-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-aws-operator 313dc3f link /test e2e-aws-operator

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.

@@ -0,0 +1,80 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this script specific for this repo of do we have it copied to multiple repos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @dmage so far the thinking is that it would be copied to multiple repos, given that we don't have a good way of sharing scripts across multiple components. It could be placed in library-go, but not all components use library-go. Also not all components use the same directories for their manifests. I'm definitely open to ideas on how to better handle this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@csrwng what about https://github.com/openshift/build-machinery-go ? cluster-image-registry-operator uses only one target from build-machinery, so it shouldn't be a problem to add a single target to other repositories.

See

include $(addprefix ./vendor/github.com/openshift/build-machinery-go/make/, \
targets/openshift/bindata.mk \
)
$(call add-bindata,assets,./bindata/...,./bindata/,assets,pkg/assets/bindata.go)

You can add something similar:

$(call add-profile-manifests,./manifests/,./profile-patches/) 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds promising, will try it out. Thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Submitted openshift/build-machinery-go#26 - will update this PR when that merges

@openshift-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 18, 2021
@csrwng
Copy link
Contributor Author

csrwng commented Feb 18, 2021

/remove-lifecycle stale

@openshift-ci-robot openshift-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 18, 2021
@csrwng csrwng force-pushed the roks_profile_patches branch from 313dc3f to fbefa25 Compare March 30, 2021 17:29
@csrwng csrwng force-pushed the roks_profile_patches branch from fbefa25 to 2f588b0 Compare March 30, 2021 17:33
@csrwng
Copy link
Contributor Author

csrwng commented Mar 30, 2021

@dmage I've now updated this PR to bump to the latest build-machinery-go and I'm now using that add a patch for ibm-cloud-managed.

@csrwng csrwng force-pushed the roks_profile_patches branch from 2f588b0 to 30c347e Compare March 30, 2021 18:50
@csrwng
Copy link
Contributor Author

csrwng commented Mar 31, 2021

/retest

@dmage
Copy link
Contributor

dmage commented Apr 1, 2021

/assign @ricardomaraschini

GOLANGCI_LINT_CACHE = $(PWD)/_output/golangci-lint-cache
GOLANGCI_LINT_VERSION = v1.24

GO_REQUIRED_MIN_VERSION = 1.14
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any special reason for not going with a newer version? I have noticed here that if it is not set we go for 1.15.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I added this here, is that verify previously failed (looks like your CI test is using an older go version):
https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-image-registry-operator/643/pull-ci-openshift-cluster-image-registry-operator-master-verify/1376950809396252672
Note that this is the minimum version. It's fine to use a newer release.

@@ -0,0 +1,86 @@
apiVersion: apps/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind giving me some explanation on how this is going to work? I mean, with this extra manifest here isn't CVO going to create two distinct deployments (07-operator.yaml and 07-operator-ibm-cloud-managed.yaml)? Is there some special handling happening @ cvo side to handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically for any given installation of openshift only one cluster profile is active. The CVO will only apply manifests that contain an annotation for the active cluster profile.
Ref: openshift/cluster-version-operator#404
Enhancement: openshift/enhancements#200

@@ -0,0 +1,3 @@
// required for gomod to pull in packages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you split this PR into two separated commits? One with vendoring stuff and the other with the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ricardomaraschini it is in 2 separate commits

@csrwng
Copy link
Contributor Author

csrwng commented Apr 6, 2021

/retest

Copy link
Contributor

@ricardomaraschini ricardomaraschini left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, ricardomaraschini

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2021
@ricardomaraschini
Copy link
Contributor

/hold

@ricardomaraschini
Copy link
Contributor

/retest

@dmage
Copy link
Contributor

dmage commented Apr 9, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2021
@dmage
Copy link
Contributor

dmage commented Apr 12, 2021

/retest

@csrwng csrwng changed the title IBM Cloud manifest profile patch Bug 1948714: IBM Cloud manifest profile patch Apr 12, 2021
@openshift-ci-robot
Copy link
Contributor

@csrwng: This pull request references Bugzilla bug 1948714, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.8.0) matches configured target release for branch (4.8.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @csrwng

Details

In response to this:

Bug 1948714: IBM Cloud manifest profile patch

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.

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 12, 2021
@openshift-ci-robot
Copy link
Contributor

@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: csrwng.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

@csrwng: This pull request references Bugzilla bug 1948714, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.8.0) matches configured target release for branch (4.8.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @csrwng

In response to this:

Bug 1948714: IBM Cloud manifest profile patch

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.

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.

@csrwng
Copy link
Contributor Author

csrwng commented Apr 12, 2021

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot removed the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Apr 12, 2021
@openshift-ci-robot
Copy link
Contributor

@csrwng: This pull request references Bugzilla bug 1948714, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.8.0) matches configured target release for branch (4.8.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @csrwng

Details

In response to this:

/bugzilla refresh

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.

@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Apr 12, 2021
@openshift-ci-robot
Copy link
Contributor

@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: csrwng.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

@csrwng: This pull request references Bugzilla bug 1948714, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.8.0) matches configured target release for branch (4.8.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @csrwng

In response to this:

/bugzilla refresh

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.

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.

@csrwng
Copy link
Contributor Author

csrwng commented Apr 15, 2021

/retest

@openshift-merge-robot openshift-merge-robot merged commit 97fbfd7 into openshift:master Apr 15, 2021
@openshift-ci-robot
Copy link
Contributor

@csrwng: All pull requests linked via external trackers have merged:

Bugzilla bug 1948714 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1948714: IBM Cloud manifest profile patch

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.

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. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants