-
Notifications
You must be signed in to change notification settings - Fork 954
feat: Add email
field to GroupMember
#1702
feat: Add email
field to GroupMember
#1702
Conversation
@svanharmelen could you please take a look? |
.gitignore
Outdated
@@ -28,3 +28,6 @@ _testmain.go | |||
*.iml | |||
*.swp | |||
*.swo | |||
|
|||
# vendor | |||
vendor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why this should be left included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this package doesn't have a vendor
directory checked into git, so excluding it is actually no-op 😉 If you do anything with a vendor directory in your code, then it makes much more sense to exclude the directory in your own gitignore file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go mod vendor -> filled in the vendor dir -> need to actually check what files you want to add instead of all...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, if it makes your life easier I'll take a PR to add this back 👍🏻
@svanharmelen I've reverted vendor change + added the typo fix in here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍🏻
I'd still prefer to go through with #1703, though... |
@svanharmelen any ETA on the release that will include this fix? |
Maybe first add a PR for the vendor stuff? Or are you good without that? |
Yep, in a minute! |
@svanharmelen #1704 is there. |
Cool, noticed on minor issue with the addition. Will merge and release when that is fixed/updated 👍🏻 |
updated |
Released |
This is required for cloudquery/cloudquery#10117.
Although the
email
attribute is only visible to group Owners for any enterprise user (see limitations), it'd be nice to have this included.Might be safely ditched in favor of #1703 (I presume #1703 is a better one)