-
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: TProxy Service Mesh Viz Beta Changes #10002
ui: TProxy Service Mesh Viz Beta Changes #10002
Conversation
@@ -90,6 +90,13 @@ | |||
@item={{item}} | |||
@oncreate={{action @oncreate item @service}} | |||
/> | |||
{{else if (and item.Intention.Allowed (not item.TransparentProxy) (eq item.Source 'specific-intention'))}} |
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 is only in the down-lines because only downstreams would have "not defined upstreams explicitly."
49d208d
to
95a1f7d
Compare
ui/packages/consul-ui/app/components/topology-metrics/source-type/index.hbs
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/mock-api/v1/internal/ui/service-topology/_
Outdated
Show resolved
Hide resolved
@@ -70,11 +86,14 @@ ${ | |||
"ChecksPassing":${fake.random.number({min: 1, max: env('CONSUL_CHECK_COUNT', fake.random.number(10))})}, | |||
"ChecksWarning":${fake.random.number({min: 0, max: env('CONSUL_CHECK_COUNT', fake.random.number(10))})}, | |||
"ChecksCritical":${fake.random.number({min: 0, max: env('CONSUL_CHECK_COUNT', fake.random.number(10))})}, | |||
"Source": "${fake.helpers.randomize(['proxy-registration', 'default-allow', 'wildcard-intention'])}", |
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 purposely left out 'specific-intention' because only downstreams would have "upstreams not explicitly defined."
@@ -184,6 +184,46 @@ components: | |||
name: Precedence | |||
asc: Ascending | |||
desc: Descending | |||
transparent-proxy: Transparent Proxy |
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.
@johncowen this file is getting really big 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.
Yeah ideally the translations for components needs to be split up to live in the components folders with all the rest of the component code. There are other things we can do to split it up also, is that something you can look at?
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 tried splitting things into folder translation/components/transparent-proxy but it didn't work. Do I have to add the new files in settings?
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 sorry, we'd have to do some broccoli magic to make the component folder thing work, there are other ways to split it up though, take a look at the docs there's some guidance there.
95a1f7d
to
818bc74
Compare
Name: '*(All Services)', | ||
Datacenter: '', | ||
Namespace: '', | ||
Intention: { |
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 had to leave this Intention block or else it creates a Deny icon.
|
||
const upstreamLines = document.getElementById('upstream-lines').getBoundingClientRect(); | ||
const upstreamColumn = document.getElementById('upstream-column').getBoundingClientRect(); | ||
this.upView = { |
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 had to make some height adjustments. I'll probably have to rework on this again for the GA dropdown.
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.
Thanks for the notes! I left a few more, I think there are 4 main things this could do with before merging:
undefined
is a bit of a weird name to use in javascript with it being so important in javascript, I can imagine various ways bugs could slip in here if something is accidentally undefined and that gets cast to a string. I think we should change this to something else so it doesn't end up getting stuck in here.- It would be good to try and avoid the multiple single use components for the notices here if possible.
- Add enough mocks so you can see all different types by default (and add the
mode
to the proxy endpoints if thats where they go) - Choose a solution for specifying paragraphs in the translations.
I think the rest could be done here, or at any point moving forwards.
Nice work!
<notice.Body> | ||
<p> | ||
{{t "components.consul.topology-metrics.notice.default-allow.body"}} | ||
</p> |
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 need to think about how we encode/describe paragraphs in the translations, if this text required two paragraphs it'd be a bit nasty to do something like:
<p>
{{t "components.consul.topology-metrics.notice.default-allow.body.p0"}}
</p>
<p>
{{t "components.consul.topology-metrics.notice.default-allow.body.p1"}}
</p>
I'd like to use a very simple markdown helper to do that (means we can encode other inline HTML also). Unless you want to take a look at that, I think in the meantime we can put very minimal HTML in the translations file, so we just end up with:
<notice.Body>
{{t "components.consul.topology-metrics.notice.default-allow.body"}}
</notice.Body>
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.
Wait, that looks the same as what I have 🤔 Did you mean using the p0
p1
?
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 need to be able to define paragraphs of text in the translations file, not in the template (in these cases). Ideally we'd use markdown in the translations file, but I guess we should add something later. So for now I'd put the <p>
tags here in the translations file.
I know this seems wrong (putting HTML in the translations file), but it would make more sense once it's markdown. To try help, imagine if we had a piece of text that has 2 paragraphs, you wouldn't want to specify each paragraph as a different translation key, which means we have to somehow specify simple formatting in the translations file. Let me know if that doesn't make sense and we can go over in a bit more detail.
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.
In this scenario I just have 1 <p>
. Use still want to use default-allow.body.p0
?
</notice.Body> | ||
<notice.Footer> | ||
<p> | ||
<a href="{{env 'CONSUL_DOCS_URL'}}/connect/registration/service-registration#upstreams" rel="noopener noreferrer" target="_blank"> |
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.
It'd also be nice to put these URLs in the translations file if you are doing 'translations everywhere'. Ideally we need to keep the root as an env var and the rest in the YAML file.
Also theres an Action component with an @external
attribute that we can use for all these links now:
<Action @href={{t "identifier" root=(env 'CONSUL_DOCS_URL')}} @external={{true}}>
{{t "identifer"}}
</Action>
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 root=(env 'CONSUL_DOCS_URL')
didn't work. Maybe we could go over this together.
@@ -184,6 +184,46 @@ components: | |||
name: Precedence | |||
asc: Ascending | |||
desc: Descending | |||
transparent-proxy: Transparent Proxy |
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.
Yeah ideally the translations for components needs to be split up to live in the components folders with all the rest of the component code. There are other things we can do to split it up also, is that something you can look at?
ui/packages/consul-ui/mock-api/v1/internal/ui/service-topology/_
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/topology-metrics/card/index.scss
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,4 @@ | |||
.topology-metrics-source-type { | |||
margin: 6px 0 6px 12px; | |||
display: table; |
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 jumps out as strange to me? I don't think we use this for any other badge type things?
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.
Yea, this is the first bad we have in the sandwiched between text. It needed a bit more spacing in the bottom and sides.
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.
Wondering if inline-block
works for what you want to do here?
f904fd7
to
5709728
Compare
5709728
to
61e0d83
Compare
75693be
to
a6168cb
Compare
@johncowen all tests have passed and I've moved this PR out of draft. Ready for last ✅ |
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, I know you are waiting on backend for this but 👍 from me!
…xy, DefaultAllow, WildcardIntention, and Source
0f6deb0
to
7038696
Compare
🍒 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/351744. |
Demo Link
✨ Created warning banner for permissive intentions (wildcard-intentions) and default-allow intentions
✨ Created topology-metrics/source-type component (label). Used in each Card.
✨ Created consul/transparent-proxy component (label). Used in service instance show page.
✨ Created warning banner for not explicitly defining an upstream intention
✨ Created a dashed line with a warning icon for card with upstream intention not explicitly defined
✨ Created a *(All Services) card as a placeholder for all the services our main service can talk to because of permissive intentions
🤡 Updated service-topology mock data with new attributes: TransparentProxy, Source, and DefaultAllow
🤡 Updated connect mock data with new attribute: Mode
♻️ Refactored the topology-metrics/card component to be in its own folder structure