Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

graphqlbackend: Add API spec for site config history diff#46205

Closed
indradhanush wants to merge 7 commits into
mainfrom
ig/site-config-diff-api
Closed

graphqlbackend: Add API spec for site config history diff#46205
indradhanush wants to merge 7 commits into
mainfrom
ig/site-config-diff-api

Conversation

@indradhanush
Copy link
Copy Markdown
Contributor

@indradhanush indradhanush commented Jan 6, 2023

Part of #46031.

What

An API to return the diff between two consecutive entries in the critical_and_site_config table which stores the entire site config as a new row each time it is updated.

Why

We want to show the history of changes to the site config in the site-admin UI as a site-admin UX improvement.

Test plan

@cla-bot cla-bot Bot added the cla-signed label Jan 6, 2023
@indradhanush indradhanush force-pushed the ig/site-config-diff-api branch from 05b4001 to baa68f3 Compare January 6, 2023 14:36
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We may be able to reuse this for code host config diff in the future, but we can cross that bridge when we get there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This can be just a string from the result of running cmp.Diff on two strings. Check out a working example of what this might look here: https://go.dev/play/p/_ajoz7jQ8xd

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also was not able to reuse type Diff because it expects a GitRevisionRange which won't work for us here.

@indradhanush indradhanush force-pushed the ig/site-config-diff-api branch from be3a664 to bc975e4 Compare January 6, 2023 15:14
@indradhanush indradhanush force-pushed the ig/persist-author-site-config branch 2 times, most recently from e601a62 to 60caff3 Compare January 9, 2023 06:15
Base automatically changed from ig/persist-author-site-config to main January 9, 2023 06:44
@indradhanush indradhanush changed the base branch from main to main-dry-run/ig/persist-author-site-config January 9, 2023 06:53
@indradhanush indradhanush changed the base branch from main-dry-run/ig/persist-author-site-config to main January 9, 2023 06:53
@indradhanush indradhanush force-pushed the ig/site-config-diff-api branch from bc975e4 to c599fa8 Compare January 9, 2023 06:54
Comment thread cmd/frontend/graphqlbackend/schema.graphql Outdated
Comment thread cmd/frontend/graphqlbackend/schema.graphql Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
A list of diffs to depict what changed since the previous version of this
EXPERIMENTAL: A list of diffs to depict what changed since the previous version of this

@indradhanush indradhanush force-pushed the ig/site-config-diff-api branch from bfc73c1 to 4c323ea Compare January 9, 2023 12:41
@indradhanush
Copy link
Copy Markdown
Contributor Author

After review, the API spec is now final. Closing this PR to move forward with implementation and follow up with a new PR.

Thank you @mrnugget!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants