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

feat: Add tag read, write, and delete commands #507

Merged
merged 3 commits into from
Jun 7, 2024
Merged

Conversation

bhavanki
Copy link
Contributor

@bhavanki bhavanki commented Jun 7, 2024

The new commands are for reading, writing, and deleting tags on the
underlying resources representing secrets.

For now, the commands are only implemented for SSM parameters.

The new commands are for reading, writing, and deleting tags on the
underlying resources representing secrets.

For now, the commands are only implemented for SSM parameters.
@bhavanki bhavanki requested a review from a team as a code owner June 7, 2024 14:18
Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 34.32836% with 176 lines in your changes missing coverage. Please review.

Project coverage is 35.97%. Comparing base (6e32e9f) to head (fe42e98).

Files Patch % Lines
cmd/tag-write.go 6.38% 44 Missing ⚠️
cmd/tag-read.go 5.26% 36 Missing ⚠️
cmd/tag-delete.go 5.71% 33 Missing ⚠️
store/awsapi_mock.go 54.16% 30 Missing and 3 partials ⚠️
cmd/root.go 0.00% 6 Missing ⚠️
store/nullstore.go 0.00% 6 Missing ⚠️
store/s3store.go 0.00% 6 Missing ⚠️
store/secretsmanagerstore.go 0.00% 6 Missing ⚠️
store/ssmstore.go 88.00% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #507      +/-   ##
==========================================
- Coverage   36.17%   35.97%   -0.20%     
==========================================
  Files          25       29       +4     
  Lines        2256     2524     +268     
==========================================
+ Hits          816      908      +92     
- Misses       1368     1538     +170     
- Partials       72       78       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@alecjacobs5401 alecjacobs5401 left a comment

Choose a reason for hiding this comment

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

Few open questions - nothing really jumps out at me immediately here otherwise

store/ssmstore.go Outdated Show resolved Hide resolved
)

func init() {
tagWriteCmd.Flags().BoolVar(&deleteOtherTags, "delete-other-tags", false, "Delete tags not specified in the command")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a descriptive variable name, but I'm curious if there is precedent for CLIs to have an argument that is semantically the same but more conventional? Maybe --reset or --sync or --prune? Really not asking for a change just opening up a discussion about what a conventional flag someone might expect to exist on a CLI that has this behavior in a low surprise manner

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 didn't really do a survey, but I did ask OpenAI for suggestions and chose this 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

Hah! Funny. I asked open AI the same thing and it suggested those (specifically sync and prune) 😂

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 guess it depends on the question phrasing and OpenAI's mood that day!

@bhavanki bhavanki merged commit 37a70e0 into master Jun 7, 2024
17 checks passed
@bhavanki bhavanki deleted the tag-commands branch June 7, 2024 20:37
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.

2 participants