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.

Sign up for GitHub

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

Jump to bottom

fix add/edit tags for policy and service #2358

Merged
merged 1 commit into from
Oct 19, 2023
Merged

fix add/edit tags for policy and service #2358

merged 1 commit into from
Oct 19, 2023

Conversation

jimmytsang
Copy link
Contributor

@jimmytsang jimmytsang commented Oct 18, 2023 •
edited
Loading

Description

Problem:

  • Users cannot add/edit policy or service tags via UI
putServiceIdentity({
  ...
  detail: {
    ...
    publicKeys: {} // Culprit, zms rdl definition expects publicKeys to be of type Array
  }
});

Contribution Checklist:

  • The pull request does not introduce any breaking changes
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

fix add/edit tags for policy and service
bb1663c 
Signed-off-by: jtsang01 <jimmy.tsang@yahooinc.com>
jimmytsang
jimmytsang commented Oct 18, 2023
View reviewed changes
ui/src/server/handlers/api.js
Comment on lines +1531 to +1539
// Note: zms expects publicKeys to be an array but we store it as an object
// Additionally, updating service tags should not modify existing public keys
let publicKeysList = [];
let publicKeysMap = params.detail.publicKeys || {};
for (const [, value] of Object.entries(publicKeysMap)) {
publicKeysList.push(value);
}
params.detail.publicKeys = publicKeysList;
}
Copy link
Contributor Author

@jimmytsang jimmytsang Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

TODO:
We currently have many instances where we incorrectly expect publicKeys as objects instead of arrays. (example) I'm proposing we address this in a follow up pr as a full fix is quite involved (and dangerous so will need lots of testing).

@jimmytsang
Copy link
Contributor Author

jimmytsang commented Oct 18, 2023 •
edited
Loading

this pr builds on top of @royelbaum existing PR #2334

@jimmytsang jimmytsang mentioned this pull request Oct 18, 2023
Closed
@AthenZ AthenZ deleted a comment from jimmytsang Oct 18, 2023
@abvaidya
Copy link
Collaborator

closes #2334

@abvaidya abvaidya merged commit 6308afd into AthenZ:master Oct 19, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chandrasekhar1996 chandrasekhar1996 chandrasekhar1996 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jimmytsang @abvaidya @chandrasekhar1996