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

release-22.2: ui: dynamically read ui data on js load #94067

Merged
merged 1 commit into from Dec 21, 2022

Conversation

dhartunian
Copy link
Collaborator

@dhartunian dhartunian commented Dec 21, 2022

Backport 1/1 commits from #94021.

/cc @cockroachdb/release


Previous work that dynamically loaded the base ui data that we attach to Window did not account for JS code which referenced the dataFromServer variable directly in top-level consts. This led to a broken login widget in the top right corner and docs links that would pin to "stable" versions instead of the specific version link of the running cluster.

This commit does not completely fix the issue but it applies several mitigations:

First, the redux store is now initialized with the loaded copy of the dataFromServer info. This ensures that login information is initialized properly. Second, the store is now constructed in the promise we define in index.tsx instead of inline in the file in which it's defined, allowing us to control execution ordering. Third, one of the dataFromServer usages in the redux login selector is moved to be inside the selector to grab values at runtime. Last, the docs.ts file is updated to no longer use consts for all the doc link strings and instead use let which allows us to reload the links at runtime after we have version data from the server.

Resolves #93273

Epic: None

Release note (ui change): Secure clusters now show correct login information in the top right corner. Docs links correctly reference the current cluster version when necessary.

Release note (bug fix): Secure clusters now show correct login information in the top right corner. Docs links correctly reference the current cluster version when necessary.

Release justification: critical bugfix for DB Console

@dhartunian dhartunian requested review from sjbarag and a team December 21, 2022 15:57
@blathers-crl
Copy link

blathers-crl bot commented Dec 21, 2022

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@sjbarag sjbarag left a comment

Choose a reason for hiding this comment

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

Oooh it applied cleanly in 22.2? Neat!

Previous work that dynamically loaded the base ui data that we attach to
`Window` did not account for JS code which referenced the `dataFromServer`
variable directly in top-level consts. This led to a broken login widget in the
top right corner and docs links that would pin to "stable" versions instead of
the specific version link of the running cluster.

This commit does not completely fix the issue but it applies several
mitigations:

First, the redux store is now initialized with the loaded copy of the
`dataFromServer` info. This ensures that login information is initialized
properly. Second, the store is now constructed in the promise we define in
`index.tsx` instead of inline in the file in which it's defined, allowing us to
control execution ordering. Third, one of the `dataFromServer` usages in the
redux login selector is moved to be inside the selector to grab values at
runtime. Last, the `docs.ts` file is updated to no longer use `const`s for all
the doc link strings and instead use `let` which allows us to reload the links
at runtime after we have version data from the server.

Resolves cockroachdb#93273

Epic: None

Release note (ui change): Secure clusters now show correct login information in
the top right corner. Docs links correctly reference the current cluster
version when necessary.

Release note (bug fix): Secure clusters now show correct login information in
the top right corner. Docs links correctly reference the current cluster
version when necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants