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

Infrastructure name #267

Merged
merged 3 commits into from
Jul 4, 2019
Merged

Infrastructure name #267

merged 3 commits into from
Jul 4, 2019

Conversation

coreydaley
Copy link
Member

@coreydaley coreydaley commented Apr 26, 2019 •
edited
Loading

Replaces #260
I hope that the few updates that I made will resolve the issues that we were seeing with #260, but the tests will tell.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 26, 2019
@coreydaley
Copy link
Member Author

/assign @dmage @adambkaplan
cc @wking

@openshift-ci-robot openshift-ci-robot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Apr 26, 2019
@coreydaley coreydaley mentioned this pull request Apr 26, 2019
Closed
@coreydaley
Copy link
Member Author

@bparees @smarterclayton Do we need to disable checking for OWNER files in the vendor directory?
#267 (comment)

@wking
Copy link
Member

wking commented Apr 26, 2019

Do we need to disable checking for OWNER files...

Or prune non-Go, see my commit message here.

@coreydaley
Copy link
Member Author

/retest

@dmage
Copy link
Contributor

dmage commented Apr 26, 2019

@coreydaley

I0426 07:29:33.590569       1 bootstrap.go:38] generating registry custom resource
E0426 07:29:33.592481       1 controller.go:240] unable to sync: unable to get storage configuration from cluster install config: infrastructures "cluster" is forbidden: User "system:serviceaccount:openshift-image-registry:cluster-image-registry-operator" cannot get resource "infrastructures" in API group "" at the cluster scope, requeuing

It's weird that in the error message the API group is empty.

@coreydaley
Copy link
Member Author

@wking Any thoughts on the error?
#267 (comment)

@coreydaley
Copy link
Member Author

coreydaley commented Apr 26, 2019 •
edited
Loading

/hold
Need to resolve this before it merges:

if infra.Status.Platform == configv1.AWSPlatformType {
	cfg.Storage.S3.Region = "us-east-1" // FIXME: installConfig.Platform.AWS.Region
}

@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 Apr 26, 2019
@coreydaley
Copy link
Member Author

Do we need to disable checking for OWNER files...

Or prune non-Go, see my commit message here.

Shouldn't need to do that, it needs to be updates for all CI/CD

@openshift-ci-robot
Copy link
Contributor

The following users are mentioned in OWNERS file(s) but are not members of the openshift org.

  • @andyxning
    • vendor/k8s.io/klog/OWNERS
  • @vincepri
    • vendor/k8s.io/klog/OWNERS
  • @dims
    • vendor/k8s.io/klog/OWNERS
  • @apelisse
    • vendor/k8s.io/kube-openapi/OWNERS
    • vendor/k8s.io/kube-openapi/OWNERS
    • vendor/k8s.io/kube-openapi/pkg/util/proto/OWNERS
  • @lavalamp
    • vendor/k8s.io/kube-openapi/OWNERS
  • @mbohlool
    • vendor/k8s.io/kube-openapi/OWNERS
    • vendor/k8s.io/kube-openapi/OWNERS
  • @seans3
    • vendor/k8s.io/kube-openapi/OWNERS
  • @jayunit100
    • vendor/k8s.io/klog/OWNERS
  • @hoegaarden
    • vendor/k8s.io/klog/OWNERS
  • @neolit123
    • vendor/k8s.io/klog/OWNERS
  • @pohly
    • vendor/k8s.io/klog/OWNERS
  • @yagonobre
    • vendor/k8s.io/klog/OWNERS
  • @detiber
    • vendor/k8s.io/klog/OWNERS

@lavalamp
Copy link

@bparees @smarterclayton Do we need to disable checking for OWNER files in the vendor directory?

Yes, you do :)

@coreydaley
Copy link
Member Author

/retest

@coreydaley
Copy link
Member Author

Blocked by openshift/api#300

@dmage dmage added this to the 4.1 milestone May 6, 2019
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. labels May 7, 2019
@wking wking mentioned this pull request May 7, 2019
Closed
@wking
Copy link
Member

wking commented May 7, 2019

openshift/installer#1725 is in flight to unblock this.

@adambkaplan
Copy link
Contributor

per @wking - we can push this to 4.2.
On upgrade we should do the following:

  1. Look for the existing openshiftClusterID tag, remove if present
  2. Add the new kubernetes.io/cluster/{infraName} tag

@wking
Copy link
Member

wking commented May 7, 2019

I dunno if you can adjust both tags in a single, atomic operation. If you can't, you'll want to add the kubernetes.io/cluster/{infraName} tag first and then remove the openshiftClusterID tag to avoid potentially leaking the bucket if you die mid-retag and the cluster is subsequently destroyed.

@coreydaley
Copy link
Member Author

@wking @adambkaplan
Is there a specific reason that we need to remove the old tag? It shouldn't hurt anything to leave it alone I would think.

@wking
Copy link
Member

wking commented May 8, 2019

Is there a specific reason that we need to remove the old tag?

Just reducing possible user confusion by removing a tag that no longer had semantic meaning.

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2019
@coreydaley
Copy link
Member Author

Blocked by openshift/installer#1725

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2019
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 2, 2019
@coreydaley
Copy link
Member Author

@dmage ptal

@coreydaley
Copy link
Member Author

/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 Jul 2, 2019
@dmage
Copy link
Contributor

dmage commented Jul 2, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 2, 2019
@coreydaley
Copy link
Member Author

/retest

@coreydaley
Copy link
Member Author

/retest
VPC limit exceeded

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2019
@coreydaley
Copy link
Member Author

/retest

3 similar comments
@coreydaley
Copy link
Member Author

/retest

@coreydaley
Copy link
Member Author

/retest

@coreydaley
Copy link
Member Author

/retest

@coreydaley coreydaley mentioned this pull request Jul 3, 2019
Merged
@coreydaley
Copy link
Member Author

@dmage ptal, tests are passing, Skipping baremetal test as we discussed.

@dmage
Copy link
Contributor

dmage commented Jul 4, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 4, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: coreydaley, dmage

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-merge-robot openshift-merge-robot merged commit 2122adf into openshift:master Jul 4, 2019
@coreydaley coreydaley mentioned this pull request Jul 5, 2019
Merged
@dmage dmage mentioned this pull request Jul 18, 2019
Closed
@coreydaley coreydaley deleted the infrastructure_name branch August 14, 2019 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adambkaplan adambkaplan Awaiting requested review from adambkaplan

@asalkeld asalkeld Awaiting requested review from asalkeld

Assignees

@dmage dmage

@adambkaplan adambkaplan

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
7 participants
@coreydaley @wking @dmage @openshift-ci-robot @lavalamp @adambkaplan @openshift-merge-robot