S3 config should not stomp local secret config by coreydaley · Pull Request #106 · openshift/cluster-image-registry-operator

coreydaley · 2018-12-01T17:18:17Z

Cluster S3 configuration should not stomp user configuration

Checks for a user supplied S3 configuration to use
If user config is not found, use the system configuration
Add a test to ensure that a user provided configuration will override the sytem provided one
Add image-registry-private-configuration-user to the README.md
Finalize S3 storage (remove bucket if managed by us)
Don't fail on S3 bucket tagging and encryption, just set the appropriate conditions to false

Fixes #79
Fixes #99

pkg/clusterconfig/clusterconfig.go

coreydaley · 2018-12-01T23:31:25Z

/retest

coreydaley · 2018-12-02T16:31:52Z

/retest

coreydaley · 2018-12-02T21:06:48Z

/retest

bparees

good start, the main thing you're missing is reacting to changed/deletion/creation of the user-defined secret.

pkg/clusterconfig/clusterconfig.go

pkg/storage/s3/s3.go

test/e2e/aws_test.go

pkg/clusterconfig/clusterconfig.go

test/e2e/aws_test.go

pkg/clusterconfig/clusterconfig.go

pkg/apis/imageregistry/v1alpha1/types.go

pkg/clusterconfig/clusterconfig.go

coreydaley · 2018-12-16T04:53:43Z

@bparees ptal

pkg/resource/generator.go

coreydaley · 2018-12-17T15:28:08Z

/retest

coreydaley · 2018-12-18T03:17:42Z

@bparees ptal
I refactored a bunch of stuff and addressed your comments.
I also merged in my other PR since I needed a few things from it in this one.

I am still working on the test, but I believe everything else is ready for another look.

pkg/operator/finalizer.go

pkg/resource/generator.go

pkg/storage/s3/s3.go

bparees

why is this PR disabling all our tests?

test/e2e/basic_empty_dir_test.go

test/e2e/unmanaged_test.go

pkg/resource/generator.go

coreydaley · 2018-12-23T05:33:18Z

@bparees ptal
This should be ready for another look, including the tests. I separated the aws-sdk-go bump into it's own commit for ease of review.

pkg/resource/podtemplatespec.go

coreydaley · 2019-01-02T18:33:47Z

/retest

Checks for a user supplied S3 configuration to use If user config is not found, use the system configuration Add a test to ensure that a user provided configuration will override the sytem provided one Add image-registry-private-configuration-user to the README.md Finalize S3 storage (remove bucket if managed by us) Don't fail on S3 bucket tagging and encryption, just set the appropriate conditions to false Fixes #79 Fixes #99

bparees · 2019-01-02T21:18:40Z

/lgtm

openshift-ci-robot · 2019-01-02T21:19:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [bparees]

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 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2018

openshift-ci-robot requested review from deads2k and dmage December 1, 2018 17:18

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 1, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2018

coreydaley commented Dec 1, 2018

View reviewed changes

pkg/clusterconfig/clusterconfig.go Show resolved Hide resolved

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 2, 2018

coreydaley changed the title ~~[WIP] S3 config should not stomp local secret config~~ S3 config should not stomp local secret config Dec 2, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 2, 2018

coreydaley assigned dmage and legionus Dec 2, 2018

coreydaley requested review from bparees and legionus and removed request for deads2k December 2, 2018 04:25

bparees requested changes Dec 3, 2018

View reviewed changes

dmage reviewed Dec 3, 2018

View reviewed changes

legionus reviewed Dec 3, 2018

View reviewed changes

pkg/clusterconfig/clusterconfig.go Show resolved Hide resolved

legionus reviewed Dec 3, 2018

View reviewed changes

pkg/apis/imageregistry/v1alpha1/types.go Outdated Show resolved Hide resolved

dmage reviewed Dec 3, 2018

View reviewed changes

pkg/clusterconfig/clusterconfig.go Outdated Show resolved Hide resolved

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 3, 2018

bparees requested changes Dec 16, 2018

View reviewed changes

pkg/resource/generator.go Outdated Show resolved Hide resolved

pkg/resource/generator.go Outdated Show resolved Hide resolved

pkg/resource/generator.go Outdated Show resolved Hide resolved

pkg/resource/generator.go Show resolved Hide resolved

bparees requested changes Dec 18, 2018

View reviewed changes

coreydaley mentioned this pull request Dec 19, 2018

Use Cluster Version ClusterID instead of Install Config ClusterID #127

Merged

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 19, 2018

bparees requested changes Dec 20, 2018

View reviewed changes

test/e2e/basic_empty_dir_test.go Outdated Show resolved Hide resolved

test/e2e/unmanaged_test.go Outdated Show resolved Hide resolved

bparees requested changes Dec 20, 2018

View reviewed changes

pkg/resource/generator.go Outdated Show resolved Hide resolved

bump github.com/aws/aws-sdk-go

af54845

bparees self-assigned this Dec 29, 2018

bparees reviewed Jan 2, 2019

View reviewed changes

pkg/resource/podtemplatespec.go Outdated Show resolved Hide resolved

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 2, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2019

openshift-merge-robot merged commit 0379f48 into openshift:master Jan 2, 2019

coreydaley deleted the github_99_s3_config_should_not_stomp_local_secret_config branch August 14, 2019 15:04

Conversation

coreydaley commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

coreydaley commented Dec 1, 2018

Uh oh!

coreydaley commented Dec 2, 2018

Uh oh!

coreydaley commented Dec 2, 2018

Uh oh!

bparees left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coreydaley commented Dec 16, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coreydaley commented Dec 17, 2018

Uh oh!

coreydaley commented Dec 18, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bparees left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coreydaley commented Dec 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

coreydaley commented Jan 2, 2019

Uh oh!

bparees commented Jan 2, 2019

Uh oh!

openshift-ci-robot commented Jan 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

coreydaley commented Dec 1, 2018 •

edited

Loading

coreydaley commented Dec 23, 2018 •

edited

Loading