-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ui: Add Consul API Gateway as an external source #11371
ui: Add Consul API Gateway as an external source #11371
Conversation
@@ -65,7 +65,7 @@ as |item index|> | |||
{{/if}} | |||
</td> | |||
</BlockSlot> | |||
{{#if (or (can "write intention" item=item) (can "view CRD intention" item=item))}} | |||
{{#if (and (or (can "write intention" item=item) (can "view CRD intention" item=item)) (not-eq item.Meta.external-source 'consul-api-gateway'))}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily hiding the actions button until we have docs and learn guides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think (at least for the moment) that intentions will ever have this meta value so you can probably remove this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support for intentions will be merged today/tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, TIL, so now I'm trying to understand why we are hiding the actions dropdown? Should I still be allowed to click on the row (I can currently). What about the actions at the bottom of the form once I click into it? Should I be able to delete and save from there if it's a consul-api-gateway intention?
Should we be showing a badge/pill in the row to show that the intention has been added/is managed via consul-api-gateway? At the moment I just don't see an actions button but with no clue as to why I don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The why is mentioned in my initial comment. Please read and remember this is temporary.
There are no create/delete actions. Please refer to the mock-ups.
I'll ask the designer about the badge/pill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool gotcha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the badge/pill. I spoke with the designer and they would like to leave that for a future release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so just so I follow here. For this release it sounds like we are not doing anything to do with intentions coming from consul-api-gateway:
- We aren't adding a badge/pill to an intention if it has api-gateway metadata?
- We temporarily hide the actions menu only here in this PR if it has api-gateway metadata, but this temporary measure will be undone once we have docs and learn guides?
Thats ^ what I understand right now, if this isn't correct or changes could you let me know.
@@ -4,9 +4,6 @@ | |||
%popover-menu-trigger { | |||
display: block; | |||
} | |||
%popover-menu-panel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this in a couple areas to see if it broken anything. I couldn't find any areas. I think width:100%
in a parent div is holding things together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems legit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before:
After:
This change makes all our menus dynamically sized to the containing text, so it essentially shrinks the widths of them all, and I don't think that's what is desired (I could be wrong?). The hardcoded width here will have been taken from the original pixel value from the original designs.
If we take the time to un-hardcode this, this should be a min width I think (we also do a weird fit-content thing somewhere that was for something similar to this, not sure this is the right tool here but thought I'd mention incase). Again, separate PR would be better, but I'm easy. (edit: but if we want to do it here, then this will need changing)
(also e2e testing/screen-component-grabbing 💯 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added back min-width
.
@extend %with-logo-consul-color-icon, %as-pseudo; | ||
} | ||
%popover-select .nomad button::before { | ||
@extend %with-logo-nomad-color-icon, %as-pseudo; | ||
} | ||
%popover-select .vault button::before { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed Vault was missing here. I was going to make it into a separate PR, but then I realized I would need to wait for it to be merged and rebase this PR.
@@ -46,6 +46,7 @@ | |||
<div class="metrics-header"> | |||
{{@service.Service.Service}} | |||
</div> | |||
{{#if (not-eq @service.Service.Meta.external-source 'consul-api-gateway')}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hiding metrics and the Configure metrics dashboard
messaging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to change this to use ember-can
so can("view dashboard for service" item=service)
or similar? ty!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this conditional works. Using ember-can
for this specific case will open this PR to other changes that will be out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realised we might need to hide the Configure dashboard
messaging in the other place that we show this button.
Q: Do you know what Kind
s could have the metadata consul-api-gateway
?
@@ -97,7 +97,7 @@ as |item|}} | |||
<h1> | |||
<route.Title @title={{item.Service.ID}} /> | |||
</h1> | |||
<Consul::ExternalSource @item={{item}} /> | |||
<Consul::ExternalSource @item={{item}} @withInfo={{true}} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied a similar implementation as Kind.
@@ -65,7 +65,7 @@ as |item index|> | |||
{{/if}} | |||
</td> | |||
</BlockSlot> | |||
{{#if (or (can "write intention" item=item) (can "view CRD intention" item=item))}} | |||
{{#if (and (or (can "write intention" item=item) (can "view CRD intention" item=item)) (not-eq item.Meta.external-source 'consul-api-gateway'))}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is our preference to always keep this kind of logic in the template or is there a point at which we prefer to move that logic into a computed property or method on the component?
(This is readable enough for now but it feels like it might be pushing the boundaries of how much logic to put into the template, if that's a thing we limit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily hiding the actions button until we have docs and learn guides.
@kaxcode In that case, would the and
logic introduced here be removed later once the docs and learn guides are available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is temporary. If needed this could be refactored.
external-source: | ||
consul-api-gateway: Consul API Gateway | ||
vault: Vault | ||
kubernetes: Kubernetes | ||
terraform: Terraform | ||
nomad: Nomad | ||
consul: Consul | ||
aws: AWS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I actually just deleted it because I remember I could use common.brand 😂
8383bdc
to
6f4c398
Compare
{{else}} | ||
Registered via {{t (concat "components.consul.external-source." externalSource)}} | ||
{{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see the previous logic for AWS
resolved by using localization, feels like a good solution that should scale as the list grows
<dl class="tooltip-panel"> | ||
<dt> | ||
<span | ||
data-test-external-source={{externalSource}} | ||
class="consul-external-source {{externalSource}}" | ||
...attributes | ||
> | ||
Registered via {{t (concat "components.consul.external-source." externalSource)}} | ||
</span> | ||
</dt> | ||
<dd> | ||
<MenuPanel @position="left" @menu={{false}}> | ||
<BlockSlot @name="header"> | ||
API Gateways manage north-south traffic from external services to services in the Datacenter. For more information, read our documentation. | ||
</BlockSlot> | ||
<BlockSlot @name="menu"> | ||
<li role="separator"> | ||
About {{t (concat "components.consul.external-source." externalSource)}} | ||
</li> | ||
<li role="none" class="learn-link"> | ||
<a tabindex="-1" role="menuitem" href={{concat (env 'CONSUL_DOCS_LEARN_URL')}} rel="noopener noreferrer" target="_blank"> | ||
Learn guides | ||
</a> | ||
</li> | ||
</BlockSlot> | ||
</MenuPanel> | ||
</dd> | ||
</dl> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary right now but if this is a pattern we see a lot of I wonder if we could make a cleanup task for the future to"componentize" this behavior (and possibly have that yield some sub-components) to help cut down on the need for manual markup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation is also done in Consul::Kind
. Making a change to how we use MenuPanel could scope creep a bit. Definitely, something to change in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I left some inline feedback here, not taken it for a spin yet but let me know what you think about the withInfo thing in the meantime.
@@ -65,7 +65,7 @@ as |item index|> | |||
{{/if}} | |||
</td> | |||
</BlockSlot> | |||
{{#if (or (can "write intention" item=item) (can "view CRD intention" item=item))}} | |||
{{#if (and (or (can "write intention" item=item) (can "view CRD intention" item=item)) (not-eq item.Meta.external-source 'consul-api-gateway'))}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think (at least for the moment) that intentions will ever have this meta value so you can probably remove this now.
@@ -96,7 +96,7 @@ ${fake.helpers.randomize([ | |||
"Precedence": ${i + 1}, | |||
${ fake.random.number({min: 1, max: 10}) > 2 ? ` | |||
"Meta": { | |||
"external-source": "${fake.helpers.randomize(['kubernetes'])}" | |||
"external-source": "${fake.helpers.randomize(['kubernetes', 'consul-api-gateway'])}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above I don't think API Gateway will add this meta, below in the other intention endpoints the same comment applies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support for intentions will be merged today/tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, TIL
ui/packages/consul-ui/tests/acceptance/dc/services/show/topology/metrics.feature
Show resolved
Hide resolved
dc: datacenter | ||
service: web | ||
--- | ||
And I don't see the "[data-test-sparkline]" element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, test all reads nicely!
Registered via {{t (concat "common.brand." externalSource)}} | ||
{{/if}} | ||
</span> | ||
{{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you were thinking here copying the the @withInfo
attribute from Consul::Kind. I think that now we have this kind of thing in multiple places it might be worthwhile changing things slightly with the multiplicity in mind.
One thing that I'm not 100% sure of here is having the decision on whether to show the info in two places. One place is here in the component (does external-source === 'consul-api-gateway'?), and the other place is where ever you use the component with the @withInfo
argument (which needs to be true). Both need to be true to show the info and the info is very specific to external-source being consul-api-gateway
. I think I follow why you need 2 'knobs' for this, but it would be good to chat through which way to do it. An additional side-note here is that if we are going to use i18n here we should use it 100% for every piece of copy, not just the odd word.
Considering all of the above, I think it would be good to think about this a little more and possibly do it differently using named slots (possibly similar thinking to evan's comment). So I'd suggest splitting this bit off into a separate PR so we can get the initial functionality merged in separately and discuss this withInfo
stuff in a bit more detail. Lemme know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'd suggest splitting this bit off into a separate PR so we can get the initial functionality merged in separately and discuss this withInfo stuff in a bit more detail. Lemme know what you think.
Would it be alright to merge things more or less as-is and make a point to take a pass at withInfo
/named slots as a separate issue, knowing that we'd need to come back and update this later? The upshot is that allows the implementation discussion around how to standardize and/or make it easier to use consistent patterns to not be under time pressure, and I'd prefer not to see this blocked on that discussion unless the conversation around what to change and implementation of said changes can be accomplished within a day or two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would guess that it could be accomplished within a day or two, but happy to merge as is if need be. It also fits in with your comment here on componentizing this a little more. My main worry would be for it to get left and then spread with the next copypaste and then it suddenly becomes a pattern by accident.
We could put this in a separate PR, merge this one pretty much immediately (yay!) and then work on this almost separate feature over the next day or so, I can't see it missing a release for spending a tiny bit more time on it. I think when I originally wrote this up in as a task we split it potentially into two tasks anyway.
I'm easy either way.
I think maybe at the very least, if we are going to use i18n we shouldn't just i18n separate words and hardcode the rest.
Oh actually, just noticed, we this extra functionality is pretty much just to provide links to docs for more info, and those links don't exist yet, so we're gonna have to come back and amend this anyway. Feels like we could just do that in one hit rather than risk this going out in a release with empty links?
Anyway, as I said I'm easy either way but obvs would prefer to split this off if it was me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if we as a group agree to circle back and address the standardization/componentizing issue as a fast follow, that's a strong enough commitment to make me comfortable with merging this as-is.
My main worry would be for it to get left and then spread with the next copypaste and then it suddenly becomes a pattern by accident.
I am happy to prioritize addressing this specific issue so that it doesn't fall into being a pattern. And even if it did, we can change it/reverse it.
I can't see it missing a release for spending a tiny bit more time on it
I'm not concerned about it missing the release; I'd like us to get into the practice of reducing the turnaround time on reviews/approvals/merges for PRs. I see this particular issue as one that isn't crucial to fix for this PR but that is important that we address separately and quickly, and we can add changes to other parts of the code that could benefit from an update as part of that work as well.
Feels like we could just do that in one hit rather than risk this going out in a release with empty links?
Is that something we've had an issue with in the past? As long as this isn't being backported to anything earlier than 1.11 I don't expect that would be a problem as docs would be required for the release to go out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could put this in a separate PR, merge this one pretty much immediately (yay!) and then work on this almost separate feature over the next day or so
Cool yeah I think we want the same outcome here, just different ways of going about it 😆
Do we want to address the i18n stuff here or later? Again, I'm easy either way.
Is that something we've had an issue with in the past?
Kind of yeah, just links not being ready and we either leave them or put placeholders in, and then they get left as they are forgotten about, and we end up with links going to 404s or the wrong page (🤔 another thing proper e2e testing could help with)
All in all, nothing here that is a specific blocker for merging as is, as long as we are coming back to anything that isn't addressed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good to merge. We have some follow-up items we've identified that can be addressed separately once this lands and that we can get done in time for the release.
Do we want to address the i18n stuff here or later?
I agree that if we are going to be using i18n/localization in the app that we need to be using it more thoroughly, however I'd like there to be a more formal discussion and decision process (as part of general team working agreements) around this before we start requiring for every part of a PR. Let's leave as-is for now but it's on my list of things for us to discuss as a group in the near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
ede3c5c
to
9beacaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I think this is good to go. I left a couple more comments/questions, but the only one that I think might be actionable in this PR is the one re: the other Configure dashboard
button. Not sure if you want to do that here or in another PR, but I'll leave an approval here anyway so its up to you.
@@ -65,7 +65,7 @@ as |item index|> | |||
{{/if}} | |||
</td> | |||
</BlockSlot> | |||
{{#if (or (can "write intention" item=item) (can "view CRD intention" item=item))}} | |||
{{#if (and (or (can "write intention" item=item) (can "view CRD intention" item=item)) (not-eq item.Meta.external-source 'consul-api-gateway'))}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so just so I follow here. For this release it sounds like we are not doing anything to do with intentions coming from consul-api-gateway:
- We aren't adding a badge/pill to an intention if it has api-gateway metadata?
- We temporarily hide the actions menu only here in this PR if it has api-gateway metadata, but this temporary measure will be undone once we have docs and learn guides?
Thats ^ what I understand right now, if this isn't correct or changes could you let me know.
@@ -46,6 +46,7 @@ | |||
<div class="metrics-header"> | |||
{{@service.Service.Service}} | |||
</div> | |||
{{#if (not-eq @service.Service.Meta.external-source 'consul-api-gateway')}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realised we might need to hide the Configure dashboard
messaging in the other place that we show this button.
Q: Do you know what Kind
s could have the metadata consul-api-gateway
?
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/500307. |
✨ Description:
This PR adds support of Consul API Gateway as an external source.
Support appears in the following areas:
Configure metrics dashboard
messaging hidden📸 Demo:
⚡ Backend Changes:
BE Support in an internal repo. The BE support has been merged.
🤡 Updates to mock-api:
Updated every endpoint that had
"external-source": "${fake.helpers.randomize(['kubernetes'])}"
.🧪 Testing:
Added an acceptance test for hiding the metrics for a Consul API Gateway.
📝 PR Tasks: