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

ui: Move peers to a subapplication #13725

Merged
merged 7 commits into from
Jul 14, 2022
Merged

ui: Move peers to a subapplication #13725

merged 7 commits into from
Jul 14, 2022

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Jul 12, 2022

Description

This PR moves the peering feature into a sub-application to take advantage of the extra features that offers.

Note there is a little more moving around/renaming to be done here going by the fact that we have decided to use the term Peering more over just Peer for code things.

All discussed offline.

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

/cc @LevelbossMike

@johncowen johncowen added theme/ui Anything related to the UI pr/no-changelog PR does not need a corresponding .changelog entry labels Jul 12, 2022
@johncowen johncowen requested a review from evrowe July 12, 2022 15:15
@github-actions github-actions bot added the type/ci Relating to continuous integration (CI) tooling for testing or releases label Jul 12, 2022
@johncowen johncowen removed the type/ci Relating to continuous integration (CI) tooling for testing or releases label Jul 12, 2022
@@ -81,7 +81,7 @@ as |key value|}}
{{#each
(get
(require '/models/peer'
path='schema'
Copy link
Contributor Author

@johncowen johncowen Jul 12, 2022

Choose a reason for hiding this comment

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

Oh forgot to mention, this missed a PR somehow for the filtering thing, I was gonna PR it separate, but it slipped in here in the copy over, I'm happy to leave it in here, but let me know if you want me to PR separate.

Copy link
Contributor

@LevelbossMike LevelbossMike left a comment

Choose a reason for hiding this comment

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

This feels a lot for making the peer route optional but 🤷 it apparently has additional benefits over that as well so 👍 - can you maybe go into detail a bit more about the additional benefits this approach provides? just for documentation purposes.

@@ -12,7 +12,7 @@ Below is a list of the most commonly used functions as bookmarklets followed by
| [Enable ACLs](javascript:Scenario('CONSUL_ACLS_ENABLE=1')) | Enable ACLs |
| [Enable TProxy](javascript:Scenario('CONSUL_TPROXY_ENABLE=1')) | Enable TProxy |
| [Enable Nspaces](javascript:Scenario('CONSUL_NSPACES_ENABLE=1')) | Enable Namespace Support |
| [Enable Peers](javascript:Scenario('CONSUL_PEERING_ENABLE=1')) | Enable Peers Support |
| [Enable Peers](javascript:Scenario('CONSUL_PEERINGS_ENABLE=1')) | Enable Peering Support |
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 need to update get-environment as well? I think this is where this cookie value is being read. And we are changing from CONSUL_PEERING_ENABLE to CONSUL_PEERINGS_ENABLE here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think I did it everywhere, let me check again just incase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed a couple 😬 I've added those and searched the codebase for references to that to triple check, thanks for the nudge!

@johncowen
Copy link
Contributor Author

a rebase has made some test weirdness here, just giving it all a once over before merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-changelog PR does not need a corresponding .changelog entry theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants