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

Bug 1535182 - adding ability to retrieve an array of sub configs #655

Merged
merged 1 commit into from Jan 18, 2018

Conversation

shawn-hurley
Copy link
Contributor

Describe what this PR does and why we need it:
Adds ability for the config package to handle an array of elements

Changes proposed in this pull request

  • Fixing registry unique name regression.
  • Adding ability to get an array of sub configs.
  • Handling the case that final object is an array will return an array
    now.
  • if not the final key, will still attempt to look into an array based on
    the name then type.

Does this PR depend on another PR (Use this to track when PRs should be merged)
depends-on

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #644

* Fixing registry unique name regression.
* Adding ability to get an array of sub configs.
* Handling the case that final object is an array will return an array
now.
* if not the final key, will still attempt to look into array based on
name then type.
@shawn-hurley shawn-hurley added bug needs-review 3.9 | release-1.1 Kubernetes 1.9 | Openshift 3.9 | Broker release-1.1 labels Jan 18, 2018
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 18, 2018
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling da5a439 on shawn-hurley:bug-1535182 into ** on openshift:master**.

Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

ACK, looks good to me.

}
names[registry.RegistryName()] = true
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Took me a while to get this, but yep it makes sense :)

Copy link
Contributor

Choose a reason for hiding this comment

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

A Set is too high level for go. Maps are much better. /s

Copy link
Contributor

@eriknelson eriknelson left a comment

Choose a reason for hiding this comment

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

Exactly what I was thinking. Made my bugfix pretty trivial, thanks 👍

@@ -449,3 +450,13 @@ func convertAPIRbacToK8SRbac(apiRole *apirbac.ClusterRole) *rbac.ClusterRole {
Rules: rules,
}
}

func validateRegistryNames(registrys []registries.Registry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: registrys -> registries

@eriknelson eriknelson merged commit 6fb44dc into openshift:master Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.9 | release-1.1 Kubernetes 1.9 | Openshift 3.9 | Broker release-1.1 needs-review size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASB support multi registry with the same name currently
5 participants