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(catalog-model): add the User entity #2587

Merged
merged 1 commit into from
Sep 25, 2020
Merged

feat(catalog-model): add the User entity #2587

merged 1 commit into from
Sep 25, 2020

Conversation

freben
Copy link
Member

@freben freben commented Sep 24, 2020

Based on discussions and suggestions in #1401 (e.g. it drops the identities and will look into using annotations/labels for that matching instead), and like with the Group entity, it models both the groups you are a direct member of and also all of the ones you transitively also are a part of.

@freben freben requested a review from a team as a code owner September 24, 2020 07:04
@freben
Copy link
Member Author

freben commented Sep 24, 2020

Whoops, was a bit eager on the button there, haven't finished writing the docs yet.

@freben
Copy link
Member Author

freben commented Sep 24, 2020

Note that the classes are still named "alpha", will rename them separately along with a changelog entry

@stefanalund
Copy link
Contributor

stefanalund commented Sep 24, 2020

@freben if I understand this correctly, we are moving to beta versioning. That is a big step and it feels a bit strange to bundle such a change with "add the User entity". What are your thoughts here?

Related issue: #2391

@freben
Copy link
Member Author

freben commented Sep 24, 2020

@stefanalund Right - my thought was that we already had support for that as the API version all over the code because we'd moved the format to beta, so this was just a matter of clarifying what was already supported. It is indeed unrelated to the issue at hand though, so I agree and will backtrack those changes.

@freben
Copy link
Member Author

freben commented Sep 24, 2020

Updated with docs and reverted versioning

@andrewthauer
Copy link
Collaborator

I'm curious on how a User would be associated with a particular login provider? Would this be inferred through the metadata.name or some future field like an annotation?

@freben
Copy link
Member Author

freben commented Sep 24, 2020

Yeah it's not super clear since it's spread out over some discussion threads. What we're thinking is that the users will have a set of well known annotations that tie them to the "primary key" of each identity provider that it has a relation to. Perhaps identity.backstage.io/google: example@gmail.com or google.com/email: example@gmail.com, but the details of that mechanism will have to be ironed out (in public) sometime soon.

This PR just lays the groundwork for it.

@stefanalund
Copy link
Contributor

We are getting a lot of questions about how one adds users and groups into Backstage. It would be great if we had some additional documentation around this. Describing them in YAML files is a good first step, even though that is not likely the default for most companies (assumption being that they would integrate an existing LDAP service, or similar).

@freben
Copy link
Member Author

freben commented Sep 24, 2020

Right, it would almost exclusively be in the form of some batch ingestion process that mirrors an actual master system. And the system probably should come with a couple of such ingestors (chiefly LDAP! and github) to get going easily. Those docs (and code) are not in scope for this change though I think.

these entries point to groups in the same namespace, so in those cases it is
sufficient to enter only the `metadata.name` field of those groups.

### `spec.directMemberOf` [required]
Copy link
Member

Choose a reason for hiding this comment

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

Thinking we should only have memberOf and let the rest be discoverable through relationships? And ofc have that be only direct membership

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure - the other one was only added as an analogue to what we already had for Group. Shall I remove the transitive one and just assume that the relationships concept solidifies in some shortish term?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think so, got an opportunity to clean up the Group kind too here


Exactly equal to `backstage.io/v1alpha1` and `User`, respectively.

### `spec.type` [required]
Copy link
Member

Choose a reason for hiding this comment

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

Can we skip this? Not sure how this would be used

Copy link
Member Author

Choose a reason for hiding this comment

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

@stefanalund any opinions from your side on type and status? postpone until we know more, or do you envision immediate use / need?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe @andrewthauer @dhenneke @Fox32 who had reactions on the above have some input too? I'm fine either way

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we would not be allowed to store user data of users that are not active (status) and we don't have a source for the type, so for me it's fine to extend that later.

On the other hand, showing that a user is not active anymore sounds quite useful. Not sure about the differentiation of the type of user.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an alternative to a type, maybe one wants to use the tags field from the metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or labels / annotations. The spec object is open to private extension too (it doesn't reject "unknown" fields per default. OK, concluded to delete both of these.

Copy link
Member

Choose a reason for hiding this comment

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

Or surface in annotations instead, sets a precedent for org-specific data too. backstage.io/user-status e.g.

Thinking it's way too early to throw standards into the mix here, and better let it grow organically through community feedback and contributions

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I've been a bit trigger happy with adding things that "seem to make sense", it is probably wise to start as small as useful but no smaller, and grow from there.

- `cook`
- `terminated`

### `spec.status` [required]
Copy link
Member

Choose a reason for hiding this comment

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

Think we should skip this too tbh, at least in the initial version

this field points to a group in the same namespace, so in those cases it is
sufficient to enter only the `metadata.name` field of that group.

### `spec.ancestors` [required]
Copy link
Member

Choose a reason for hiding this comment

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

Right, didn't realize these were here. So we're thinking the same thing here right? Remove from the spec, keeping only parent for now, and then modelling with relationships instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! But that's gonna be in a separate PR.

@freben
Copy link
Member Author

freben commented Sep 24, 2020

@Rugvip updated in a much slimmed down version!

Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

:shipit:

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.

None yet

6 participants