Skip to content
This repository was archived by the owner on Jul 24, 2019. It is now read-only.

KS endpoint type lookup for Keystone, Cinder & Glance Charts - #240

Merged
v1k0d3n merged 7 commits into
att-comdev:masterfrom
portdirect:2.0ß/ks-endpoint-normalisation
Mar 4, 2017

Hidden character warning

The head ref may contain hidden characters: "2.0\u00df/ks-endpoint-normalisation"
Merged

KS endpoint type lookup for Keystone, Cinder & Glance Charts#240
v1k0d3n merged 7 commits into
att-comdev:masterfrom
portdirect:2.0ß/ks-endpoint-normalisation

Conversation

@intlabs

@intlabsintlabs commented Mar 2, 2017

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?: To implement Keystone endpoint lookups by type for the Keystone & Cinder Charts

What issue does this pull request address?: Superceeds #208

Notes for reviewers to consider:
This is part of a series of PR's that will change each chart to use the Endpoint Type lookup method over the Endpoint Name for Keystone Endpoints.

Specific reviewers for pull request:
@alanmeadows@v1k0d3n@wilkers-steve@larryrensing@gardlt

@intlabsintlabs changed the title 0.2.0ß/ks endpoint normalisationImplement Keystone endpoint lookups by type for the Keystone ChartMar 2, 2017
@intlabsintlabs changed the title Implement Keystone endpoint lookups by type for the Keystone ChartKS endpoint lookups by type for Keystone ChartMar 2, 2017
@intlabsintlabs changed the title KS endpoint lookups by type for Keystone ChartKS endpoint lookups by type for Keystone & Cinder ChartsMar 2, 2017
@intlabs

intlabs commented Mar 2, 2017

Copy link
Copy Markdown
ContributorAuthor

@alanmeadows If you could review at this stage it would be really appreciated.

apiVersion: v1
kind: Service
metadata:
name: {{ .Values.service.api.name }}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Though I would like the service name to be configurable, I removed that option here to be consistent with other charts.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think that's fine. we can address/discuss once we're in openstack.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed here

@v1k0d3nv1k0d3n left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@intlabs looks go to me. once we get @alanmeadows 'ok' i'd like to get this in and documented before importing into OpenStack.

apiVersion: v1
kind: Service
metadata:
name: {{ .Values.service.api.name }}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think that's fine. we can address/discuss once we're in openstack.

@intlabs

intlabs commented Mar 2, 2017

Copy link
Copy Markdown
ContributorAuthor

@v1k0d3n / @alanmeadows I'm fine with either merging this PR and then hitting the follow up charts in a series of rapid PRs before we move, or accumulating all of this work under this one. Though before progressing on either path I'd like to ensure that the work to date makes the grade :)

@v1k0d3n

Copy link
Copy Markdown
Collaborator

since @alanmeadows is out, @wilkers-steve and @larryrensing can you review this as well?

@intlabs as long as we have commitment to keep driving these child PR's (please submit an Issue, as a show of intent), then I'm fine with pulling this in as is...just need the commitment.

@intlabs

intlabs commented Mar 2, 2017

Copy link
Copy Markdown
ContributorAuthor

@v1k0d3n, I'll see it through, to the sweet ( and hopefully not bitter! ) end. I should have Glance ready to go this evening (GMT), with the remainder following tomorrow. Cheers, and thanks for your patience on this. :)

@intlabsintlabs changed the title KS endpoint lookups by type for Keystone & Cinder ChartsKS endpoint type lookup for Keystone, Cinder & Glance ChartsMar 3, 2017
@v1k0d3n

v1k0d3n commented Mar 4, 2017

Copy link
Copy Markdown
Collaborator

@larryrensing or @wilkers-steve: i know you're getting a 500 when using the "Commits" tab, but can you review by selecting the "Files Changed" tab as a workaround? There are 17 files committed in this PR (at the moment), and we can still review and get merged. From the "Files Changed" tab, you can still select individual commits by using the drop-down "Changes from x commits".

I think @intlabs and I are saying that things look good, but I'd like to get another set of eyes on it formally.

btw: an issue has been submitted with the github team. i'm sure that this is a bug related to their UI changes this past week.

@wilkers-stevewilkers-steve left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clicking the "files changed" tab is what's giving the 500.

Pete experienced the same issue with the tab, but pointed out that the mobile site works fine. From what I can see, this looks fine. LGTM

@larryrensinglarryrensing left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@v1k0d3n

Copy link
Copy Markdown
Collaborator

@wilkers-steve strange, but thanks for checking it out. hopefully we don't run into this issue again.

thanks @intlabs! nice cleanup.

@v1k0d3n
v1k0d3n merged commit 6e17d3d into att-comdev:masterMar 4, 2017
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@intlabs@v1k0d3n@wilkers-steve@larryrensing