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

Only copy credentials for the cloud used to deploy OCP #854

Merged
merged 2 commits into from
Dec 13, 2018
Merged

Only copy credentials for the cloud used to deploy OCP #854

merged 2 commits into from
Dec 13, 2018

Conversation

flaper87
Copy link
Contributor

We should copy only the cloud enabled by OS_CLOUD, rather than copying the entire clouds.yaml available in the node that runs openshift-install.

In addition, this PR switches to use go-yaml rather than ghodss as the former does not serialize the map keys properly. We should use go-yaml, which is the same used by clientconfig

/cc @hardys @russellb

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 10, 2018
@crawford
Copy link
Contributor

/hold

We explicitly switched from go-yaml before (cc @abhinavdahiya). We are currently trying to get a critical set of patches in.

@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 Dec 10, 2018
@abhinavdahiya
Copy link
Contributor

In addition, this PR switches to use go-yaml rather than ghodss as the former does not serialize the map keys properly. We should use go-yaml, which is the same used by clientconfig

Any examples?

more info on why k8s uses ghodss https://github.com/kubernetes-sigs/yaml

@flaper87
Copy link
Contributor Author

In addition, this PR switches to use go-yaml rather than ghodss as the former does not serialize the map keys properly. We should use go-yaml, which is the same used by clientconfig

Any examples?

more info on why k8s uses ghodss https://github.com/kubernetes-sigs/yaml

So, the config file generated by this would not serialize the keys as clientconfig expects them. The output is something like:

clouds:
    openstack:
        AuthUrl: # instead of auth_url
        Username: # instead of username

If there's something that one can set in ghodss to make it work as clientconfig expects, then I'm all for it. I may just lack of go-foo 😄

@flaper87
Copy link
Contributor Author

/test e2e-aws

@abhinavdahiya
Copy link
Contributor

So looking at https://github.com/gophercloud/utils/blob/34f5991525d116b3832e0d9409492274f1c06bda/openstack/clientconfig/results.go

ClientConfig is only yaml serializable. github.com/ghodss/yaml uses the json tags to drive serialization to both json and yaml.
So two options:

  1. update github.com/gophercloud/utils/openstack/clientconfig.ClientConfig to include json tags...
  2. add second import gopkg.in/yaml.v2 just for ClientConfig

@flaper87
Copy link
Contributor Author

@abhinavdahiya thanks a lot for the info. I'll work with the gophercloud team on adding the json tags for these structs.

Can we go with option 2 until the work on clientconfig is done? I'll put a task on me to clean this up as soon as the work in clientconfig lands

@abhinavdahiya
Copy link
Contributor

@abhinavdahiya thanks a lot for the info. I'll work with the gophercloud team on adding the json tags for these structs.

Can we go with option 2 until the work on clientconfig is done? I'll put a task on me to clean this up as soon as the work in clientconfig lands

i'm fine with option 2 😇

@flaper87
Copy link
Contributor Author

cancelling the hold since @abhinavdahiya is ok with option 2 while I work on moving clientconfig out of go-yaml

/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 Dec 13, 2018
@abhinavdahiya
Copy link
Contributor

/hold

The go-yaml repo suggests gopkg.in/yaml.v2 import . https://github.com/go-yaml/yaml/blob/v2/README.md#api-documentation

@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 Dec 13, 2018
@flaper87
Copy link
Contributor Author

@abhinavdahiya oh, totally missed that. Thanks!

/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 Dec 13, 2018
hardys
hardys reviewed Dec 13, 2018
View reviewed changes
pkg/asset/manifests/openshift.go
@@ -76,10 +83,15 @@ func (o *Openshift) Generate(dependencies asset.Parents) error {
},
}
case "openstack":
clouds, err := clientconfig.LoadCloudsYAML()
opts := new(clientconfig.ClientOpts)
cloud, err := clientconfig.GetCloudFromYAML(opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to modify https://github.com/openshift/installer/blob/master/pkg/asset/installconfig/openstack/openstack.go#L24 in a similar way?

Copy link
Contributor Author

@flaper87 flaper87 Dec 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

No, this is only relevant for the marshal step. The install config step should work just fine as it's all being done through clientconfig

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack thanks for clarifying

@hardys
Copy link
Contributor

hardys commented Dec 13, 2018

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flaper87, hardys

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 23aa436 into openshift:master Dec 13, 2018
@wking wking mentioned this pull request Dec 14, 2018
Merged
flaper87 added a commit to flaper87/utils that referenced this pull request Jan 24, 2019
@flaper87
Add json tags to clientconfig's structs
116170b 
Some yaml libraries use json tags to drive serialization for both, json
and yaml. This patch adds json tags to the clientconfig's structs to
support those libraries.

The lack of these tags has caused some integration issues with other
softare using packages like `ghodss/yaml`. An example of this can be
found in the PR linked below:

openshift/installer#854
@flaper87 flaper87 mentioned this pull request Jan 24, 2019
Merged
jtopjian pushed a commit to gophercloud/utils that referenced this pull request Jan 24, 2019
@flaper87 @jtopjian
Add json tags to clientconfig's structs (#75)
9c3b9f2 
* Add json tags to clientconfig's structs

Some yaml libraries use json tags to drive serialization for both, json
and yaml. This patch adds json tags to the clientconfig's structs to
support those libraries.

The lack of these tags has caused some integration issues with other
softare using packages like `ghodss/yaml`. An example of this can be
found in the PR linked below:

openshift/installer#854

* Make format happy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hardys hardys hardys left review comments

@russellb russellb Awaiting requested review from russellb

Assignees

@hardys hardys

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
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@flaper87 @crawford @abhinavdahiya @hardys @openshift-ci-robot @openshift-merge-robot