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.

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

Add simple completion of GCS configuration #11

Merged
merged 2 commits into from
Sep 19, 2018

Conversation

legionus
Copy link
Contributor

No description provided.

@legionus legionus requested a review from dmage September 19, 2018 15:34
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 19, 2018
@bparees
Copy link
Contributor

bparees commented Sep 19, 2018

@legionus please split your vendoring commit out to make this reviewable.

@coreydaley fyi, you should compare notes w/ what you're doing for s3 to make sure we have the same general pattern/flow.

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>
@legionus
Copy link
Contributor Author

@bparees Done

}
cr.Spec.HTTPSecret = fmt.Sprintf("%x", string(secretBytes[:]))

logrus.Warn("No HTTP secret provided - generated random secret")
Copy link
Contributor

Choose a reason for hiding this comment

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

How should it be provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmage Thanks! Removed.

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>
@coreydaley
Copy link
Member

@bparees will do

if _, err := rand.Read(secretBytes[:]); err != nil {
return fmt.Errorf("could not generate random bytes for HTTP secret: %s", err)
}
cr.Spec.HTTPSecret = fmt.Sprintf("%x", string(secretBytes[:]))
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.docker.com/registry/configuration/#http

A random piece of data used to sign state that may be stored with the client to protect against tampering. For production environments you should generate a random piece of data using a cryptographically secure random generator. If you omit the secret, the registry will automatically generate a secret when it starts. If you are building a cluster of registries behind a load balancer, you MUST ensure the secret is the same for all registries.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok thanks

return err
}

err = client.Bucket(d.Config.Bucket).Create(ctx, projectID, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

do you not need to check if the bucket already exists?

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 is why it's a simple gcs completion. I do not know how to do it better ))

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. So you're still investigating how to check if a bucket exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bparees Yes, I wanted to leave it as follow up. This PR is just a preview to understand how to update the configuration, when to do it, and how the function might looks like.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's reasonable

@bparees bparees self-assigned this Sep 19, 2018
@bparees
Copy link
Contributor

bparees commented Sep 19, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 297e064 into openshift:master Sep 19, 2018
wking added a commit to wking/cluster-image-registry-operator that referenced this pull request Apr 23, 2019
Generated with:

  $ dep ensure

using:

  $ dep version
  dep:
   version     : v0.5.1
   build date  : 2019-03-20
   git hash    : faa61893
   go version  : go1.10.3
   go compiler : gc
   platform    : linux/amd64
   features    : ImportDuringSolve=false

I'm not sure why there are so many changes.  For example, the
go.opencensus.io stuff is originally from a531f87 (Update vendor,
2018-09-19, 2018-09-19, openshift#11), where it was used by
vendor/cloud.google.com and vendor/google.golang.org/api/transport.
Those consumers are still there, but are apparently unused by the
operator, because I can build the operator without issues after
removing them.  I expect this project wants a:

  [prune]
    non-go = true
    go-tests = true
    unused-packages = true

or similar in Gopkg.toml, but am punting on that for now.
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. 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
Development

Successfully merging this pull request may close these issues.

6 participants