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

rpk profile double pointer fixes #17170

Merged
merged 3 commits into from
Mar 18, 2024
Merged

rpk profile double pointer fixes #17170

merged 3 commits into from
Mar 18, 2024

Conversation

twmb
Copy link
Contributor

@twmb twmb commented Mar 18, 2024

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x

Release Notes

  • none

When we made an auth current, we reordered the underlying auth slice
which is _not_ pointer based.

Any changes to the pointer fields _after_ reodering were changes to
an old slice that would no longer be saved.

Now, we MakeAuthCurrent with two pointer indirects --
The underlying pointer to the slice element is used for the invariant
check, did we find this auth.
We now update the pointer-to-the-pointer so that if we update any fields
_after_ MakeAuthCurrent, they're mirrored into the new slice.
For the same reasons as the prior commit.

Only `rpk profile create` actually modify the profile fields after
the move, and only in `rpk profile create --from-cloud`.
gene-redpanda
gene-redpanda previously approved these changes Mar 18, 2024
Copy link
Contributor

@gene-redpanda gene-redpanda left a comment

Choose a reason for hiding this comment

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

This is technically correct stuff, but double pointers? Not a fan.

We were updating the virtual yaml, but not the actual yaml. We want to
update the actual so that when a save happens, the version is bumped.
Copy link
Contributor

@jan-g jan-g left a comment

Choose a reason for hiding this comment

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

Tested:

  • with old profile vrsion
  • with new profile, outdated token
  • new profile, no token
  • new profile, unexpored token
  • expired token, when ^Cing out rather than picking a cluster

@twmb
Copy link
Contributor Author

twmb commented Mar 18, 2024

Bypassing test suite due to extensive local testing, and existing tests don't cover changes here (which is definitely something to work on as a follow up)

@twmb twmb merged commit 06c5726 into redpanda-data:dev Mar 18, 2024
16 of 19 checks passed
@twmb twmb deleted the profile-fix2 branch March 18, 2024 18:02
@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Mar 18, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants