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(proposals): add nested API objects #639

Merged
merged 10 commits into from
Aug 14, 2024

Conversation

jbrockopp
Copy link
Contributor

@jbrockopp jbrockopp commented Jul 1, 2022

Co-authored-by: @ecrupper

Please vote for which option you prefer the most:

@jbrockopp jbrockopp self-assigned this Jul 1, 2022
@jbrockopp jbrockopp changed the title feat(proposals): start nested API objects feat(proposals): add nested API objects Jul 1, 2022
@jbrockopp jbrockopp marked this pull request as ready for review July 1, 2022 16:49
@jbrockopp jbrockopp requested a review from a team as a code owner July 1, 2022 16:49
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

I prefer option 1 as Vela has not released v1 yet, so, based on semantic versioning, API changes should be expected.

+ "owner": {
+ "id": 1,
+ "name": "OctoKitty",
+ "favorites": ["github/octocat"],
Copy link
Member

@cognifloyd cognifloyd Jul 1, 2022

Choose a reason for hiding this comment

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

I don't think favorites should be exposed in most cases. It seems to me that that's a user preference that the UI (or CLI) should be able to request for the current user, but nobody/nothing else needs it.

What purpose does exposing a user's favorites to a build (in env vars) or in these API calls have? Is there a use case I haven't considered?

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 don't think we necessarily meant to propose that the owner field has to have the favorites displayed.

More so, I think we simply took the existing user API definition:

// User is the library representation of a user.
//
// swagger:model User
type User struct {
	ID           *int64    `json:"id,omitempty"`
	Name         *string   `json:"name,omitempty"`
	RefreshToken *string   `json:"-"`
	Token        *string   `json:"-"`
	Hash         *string   `json:"-"`
	Favorites    *[]string `json:"favorites,omitempty"`
	Active       *bool     `json:"active,omitempty"`
	Admin        *bool     `json:"admin,omitempty"`
}

And removed the fields that can't be returned (- in JSON field tags).

Regarding implementation, we can certainly ensure that favorites is not returned to minimize payload size.

@cognifloyd
Copy link
Member

cognifloyd commented Aug 19, 2022

Here's an interesting approach to API compatibility guarantees:
Vault clarifies that /v1 is not finalized yet in the docs:

Backwards compatibility: At the current version, Vault does not yet promise backwards compatibility even with the v1 prefix. We'll remove this warning when this policy changes. At this point in time the core API (that is, sys/ routes) change very infrequently, but various secrets engines/auth methods/etc. sometimes have minor changes to accommodate new features as they're developed.

https://www.vaultproject.io/api-docs

We could copy that approach and add a note to the docs about compatibility.

@chrispdriscoll
Copy link
Contributor

Per monthly review of open PRs, the Vela Admins will keep this proposal open until PRs are raised.

@plyr4
Copy link
Contributor

plyr4 commented Aug 14, 2024

gorm preload came in clutch

Copy link
Contributor

@KellyMerrick KellyMerrick left a comment

Choose a reason for hiding this comment

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

lgtm

@ecrupper ecrupper merged commit 4aa740d into main Aug 14, 2024
@ecrupper ecrupper deleted the feature/proposal/nested_api_objects branch August 14, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates a change to the API area/server Indicates a change to the server enhancement Indicates an improvement to a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants