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

Remove display name #33

Closed
hausdorff opened this issue Jul 22, 2019 · 9 comments
Closed

Remove display name #33

hausdorff opened this issue Jul 22, 2019 · 9 comments
Labels
feature/pac Impacts the Policy as Code offering kind/enhancement Improvements or new features resolution/fixed This issue was fixed

Comments

@hausdorff
Copy link
Contributor

Following up from this issue: #21 (comment)

I think we should remove displayName from the PaC policy model, and just use name and description instead. See discussion below.

is there a way we can generate these names for users from a display name? Or are we forgoing to the display name?

We can generate IDs from display names, but I personally think we should just get rid of the display name field—I think it's unlikely that having a dedicated "display name" field that separate from ID will increase clarity or decrease complexity:

  • Users are already used to providing "user readable" IDs to all Pulumi CustomResource classes, and that seems to work ok.
  • AWS config rules don't have display names, and that seems to work ok too (cf.).
  • If IDs are generated from display names, that just means that the display names have to be unique instead of the IDs—effectively making the display name the unique ID. And if that's true, why not just eliminate the ID field altogether?

cc @ekrengel

@hausdorff hausdorff added feature/pac Impacts the Policy as Code offering pac/mvp labels Jul 22, 2019
@ekrengel
Copy link
Contributor

@hausdorff is it currently included in the policy model and just not used in the examples you have?

We can remove it from the Policy model and then at a later point remove it from the service once we have time.

@hausdorff
Copy link
Contributor Author

It's not currently included—I'd meant to add it once we had end-to-end examples, but in writing them I began to believe they were basically totally redundant, and watching @clstokes write little helper functions to make it more concise to define policies convinced me this is baggage we don't need. So, that's why now I'm proposing we just don't expose them.

@ekrengel
Copy link
Contributor

To clarify, we would keep the display name for Policy Packs -- this would only be for Policies?

@cnunciato @chrsmith Any thoughts on removing the display name for the policies?

@cnunciato
Copy link
Member

I wouldn't argue strongly for it or anything, but since name is also a unique key, it's going to be a tad less readable/usable than a display name would, and (IIUC) slightly odd not to have a display name at the pack level, but to have one at the policy level. And since we do use display names in several other places in the app, it seems reasonable to me to offer them here, too.

That said, if it's a bad fit, it's the kind of thing that could easily be added later.

@chrsmith
Copy link
Contributor

I definitely think we should keep them. We can make the display name optional, and if not provided just display the more constrained name field. But I can see a world where a policy pack that has a lot of time/energy spent on it would want to provide a user-friendly name instead of s3-no-public-acls. And given that we've already wired the display name field through various APIs and our storage layer, it'd be more trouble to remove them than just make it optional for the time being. (And if after beta we are still convinced it isn't of any use, we can revisit this.)

@ekrengel
Copy link
Contributor

And if after beta we are still convinced it isn't of any use, we can revisit this.

I think this is a good idea! @hausdorff agree?

@hausdorff
Copy link
Contributor Author

This issue is meant to track how users write policy packs—all I'm proposing is that we don't include "display name" in the PolicyPack API. We can remove them from the service later if we like, too, but I don't think we should expose it to the user through the service API or the SDK.

@hausdorff
Copy link
Contributor Author

I wouldn't argue strongly for it or anything, but since name is also a unique key, it's going to be a tad less readable/usable than a display name would, and (IIUC) slightly odd not to have a display name at the pack level, but to have one at the policy level. And since we do use display names in several other places in the app, it seems reasonable to me to offer them here, too.

That said, if it's a bad fit, it's the kind of thing that could easily be added later.

Eh, we have an existence proof that making the ID the name is clear enough (cf., AWS upstream), and none of our CustomResource implementations have separate notions of "display name", so this is actually more consistent with how we already do things everywhere else. IMO anyway.

Other policy tools (including AWS upstream) implement this without a notion of display name The feedback we got from elsewhere indicates that our biggest usabilitissues are (1)

@ekrengel ekrengel removed the pac/mvp label Aug 7, 2019
@justinvp justinvp added resolution/fixed This issue was fixed kind/enhancement Improvements or new features labels Jul 18, 2023
@justinvp
Copy link
Member

We've already removed display name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/pac Impacts the Policy as Code offering kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Projects
None yet
Development

No branches or pull requests

5 participants