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

IR-91: Support Azure CloudName #578

Merged
merged 2 commits into from Jul 20, 2020
Merged

IR-91: Support Azure CloudName #578

merged 2 commits into from Jul 20, 2020

Conversation

dmage
Copy link
Member

@dmage dmage commented Jul 13, 2020

No description provided.

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

dmage commented Jul 13, 2020

/assign @ricardomaraschini

@dmage
Copy link
Member Author

dmage commented Jul 15, 2020

/retest

@dmage
Copy link
Member Author

dmage commented Jul 15, 2020 

failed to wait for the imageregistry resource to be processed: Get https://api.ci-op-vh65zrbi-0ca04.ci.azure.devcluster.openshift.com:6443/apis/imageregistry.operator.openshift.io/v1/configs/cluster: read tcp 172.16.175.114:41780->52.154.154.117:6443: read: connection timed out

/retest

@dmage
Copy link
Member Author

dmage commented Jul 16, 2020

/retest

ricardomaraschini
ricardomaraschini suggested changes Jul 16, 2020
View reviewed changes
go.mod
@@ -4,11 +4,12 @@ go 1.13

require (
cloud.google.com/go v0.40.0
github.com/Azure/azure-pipeline-go v0.2.2 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have this squashed into previous commit (vendor)?

Copy link
Member Author

Choose a reason for hiding this comment

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

commits are reordered, go.mod is a part of the first commit

pkg/storage/azure/azure.go Outdated
@@ -235,34 +249,69 @@ type driver struct {
Config *imageregistryv1.ImageRegistryConfigStorageAzure
KubeConfig *rest.Config
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 "KubeConfig" still in use?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, removed

pkg/storage/azure/azure.go

authorizer autorest.Authorizer
sender autorest.Sender
httpSender pipeline.Factory
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 add a comment here indicating that these three are used solely for tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

added

pkg/storage/azure/azure.go Outdated
@@ -302,36 +361,17 @@ func (d *driver) VolumeSecrets() (map[string]string, error) {
}

// containerExists determines whether or not an azure container exists
func (d *driver) containerExists(containerName string) (bool, error) {
if d.Config.AccountName == "" || d.Config.Container == "" {
func containerExists(ctx context.Context, environment autorestazure.Environment, accountName string, key string, containerName string) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

accountName, key, containerName string

vendor/github.com/Azure/go-autorest/autorest/mocks/LICENSE
@@ -0,0 +1,191 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have this squashed into previous commit (vendor)?

vendor/modules.txt
@@ -24,6 +24,8 @@ github.com/Azure/go-autorest/autorest/azure/auth
github.com/Azure/go-autorest/autorest/azure/cli
# github.com/Azure/go-autorest/autorest/date v0.2.0
github.com/Azure/go-autorest/autorest/date
# github.com/Azure/go-autorest/autorest/mocks v0.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have this squashed into previous commit (vendor)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's part of the vendor commit

pkg/storage/azure/azure_test.go
sender.AppendResponse(mocks.NewResponseWithContent(`?`))
sender.AppendResponse(mocks.NewResponseWithContent(`{"name":"account"}`))
sender.AppendResponse(mocks.NewResponseWithContent(`{"keys":[{"value":"firstKey"}]}`))
sender.AppendResponse(mocks.NewResponseWithContent(`{"keys":[{"value":"firstKey"}]}`))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use AppendAndRepeatResponse here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we should eliminate the second call and I'd like to have the second call recorded explicitly

@dmage dmage force-pushed the azure-govcloud branch 2 times, most recently from cc305ed to 3026ec1 Compare July 16, 2020 14:10
@dmage
Copy link
Member Author

dmage commented Jul 16, 2020

@ricardomaraschini comments are addressed

@dmage
Copy link
Member Author

dmage commented Jul 16, 2020

AWS is not affected by this PR.
/test e2e-aws-image-registry
/test e2e-aws-upgrade

@dmage
bump(*)
9585c8f
@dmage
Support Azure CloudName
7db4de2 
Possible values:

 *  AzurePublicCloud
 *  AzureUSGovernmentCloud
 *  AzureChinaCloud
 *  AzureGermanCloud

When the storage is not configured, the value is obtained from the
Infrastructure object.
@ricardomaraschini
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [dmage,ricardomaraschini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

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

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@dmage
Copy link
Member Author

dmage commented Jul 17, 2020

/hold
aws problems

@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 Jul 17, 2020
@dmage
Copy link
Member Author

dmage commented Jul 20, 2020

/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 20, 2020
@openshift-bot
Copy link
Contributor

/retest

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

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@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 bd2ed7e into openshift:master Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ricardomaraschini ricardomaraschini ricardomaraschini requested changes

@bparees bparees Awaiting requested review from bparees

Assignees

@ricardomaraschini ricardomaraschini

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
@dmage @ricardomaraschini @openshift-ci-robot @openshift-bot @openshift-merge-robot