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

fix(cli): outputs unique list of grants for each dest #1140

Merged
merged 1 commit into from
Mar 14, 2022
Merged

Conversation

kimskimchi
Copy link
Contributor

Summary

Checklist

  • Wrote appropriate unit tests - CLI: setup tests #1129
  • Considered security implications of the change
  • Updated associated docs where necessary
  • Updated associated configuration where necessary
  • Change is backwards compatible if it needs to be (user can upgrade without manual steps?)
  • Nothing sensitive logged
  • Commit message conforms to Conventional Commit

Related Issues

Resolves #1071

internal/cmd/list.go Show resolved Hide resolved
@@ -55,10 +56,14 @@ func list() error {
}
}

gs := make(map[string]string)
gs := make(map[string]mapset.Set)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can get similar functionality to this using a map of maps without adding a new dependency here. People use maps with a boolean value as the key to get set functionality in Go (yes, it is a bit ugly haha).

gs := make(map[string]map[string]bool)
for _, g := range grants {
	privileges, ok := gs[g.Resource]
	if !ok {
		privileges = make(map[string]bool)
		gs[g.Resource] = privileges
	}

	privileges[g.Privilege] = true
}

Then for the privileges you can iterate through the keys in the nested privileges maps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, it was a bit ugly and thought it would be re-used in a bunch of places in the future. Is it better to avoid this dependency?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion yeah, I think most Go devs would understand something like the example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I was digging into this for a bit actually (should I implement myself VS use the library), maybe for a bit too long 😅 , and these were the reasons for my choice:

  1. Set is a common struct that gets used a lot (in a lot of other languages), so most people understand the library itself
  2. This dependency is a highly used one (https://github.com/deckarep/golang-set), thus heavily tested in major companies (Docker, 1Password, etc)
  3. It was much easier to read (especially because mine was a nested set)
  4. efficient: I end up coding very little (eg. print, which is also super useful to call when debugging)
  5. Implementing functions for sets myself would be more prone to error vs using a widely used/tested library that's 8 yrs old >_<
  6. and... most importantly: I wasn't convinced yet of the counter-arguments (including [0]), as the pros of using a library seemed to outweigh the cons

[0] https://stackoverflow.com/questions/34018908/golang-why-dont-we-have-a-set-datastructure

(apologies for long reply but this was my research 😛)

@kimskimchi kimskimchi merged commit 158bc9c into main Mar 14, 2022
@kimskimchi kimskimchi deleted the cli-twice branch March 14, 2022 16:46
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.

infra list lists access twice
2 participants