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

Add a relationships read command #50

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

josephschorr
Copy link
Member

No description provided.

@josephschorr josephschorr changed the title Add a read relationships command Add a read relationships command Oct 15, 2021
jakedt
jakedt previously requested changes Oct 15, 2021
RelationshipFilter: readFilter,
}
if zedtoken := cobrautil.MustGetString(cmd, "revision"); zedtoken != "" {
request.Consistency = &v1.Consistency{
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be snapshot?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should it? What is the most likely "expected' behavior?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is reasonable until we address #37

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm honestly not 100% sure which is the preferred solution, but I think its better to just use the at least as for now, since you'll likely want to see the state of the world at which a Check would operate

Copy link
Member

Choose a reason for hiding this comment

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

read in zanzibar is snapshot... which is consistent with v0

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but that's not the default now in V1 (I believe)

cmd/zed/relationship.go Outdated Show resolved Hide resolved
Comment on lines +135 to +129
request := &v1.ReadRelationshipsRequest{
RelationshipFilter: readFilter,
}
Copy link
Member

Choose a reason for hiding this comment

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

style nitpick, this could be one line

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: Joseph Schorr <josephschorr@users.noreply.github.com>
@jzelinskie jzelinskie added area/CLI Affects the command line area/tooling Affects the dev or user toolchain priority/2 medium This needs to be done labels Oct 15, 2021
@jzelinskie jzelinskie linked an issue Oct 15, 2021 that may be closed by this pull request
@jzelinskie jzelinskie changed the title Add a read relationships command Add a relationships read command Oct 15, 2021
@jakedt jakedt dismissed their stale review October 18, 2021 20:24

We can wait until the follow-up for the requested changes.

@josephschorr josephschorr merged commit f744d01 into authzed:main Oct 18, 2021
@josephschorr josephschorr deleted the read-rel-cmd branch October 18, 2021 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CLI Affects the command line area/tooling Affects the dev or user toolchain priority/2 medium This needs to be done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement zed relationship read
3 participants