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

[cli] add a plan step to push perms #556

Merged
merged 5 commits into from
Nov 27, 2024
Merged

[cli] add a plan step to push perms #556

merged 5 commits into from
Nov 27, 2024

Conversation

stopachka
Copy link
Contributor

When we pushed perms, previously we would say: "This will overwrite your production perms. Ok?"

But that's a bit scary, as you can't really tell what has changed.

In this PR, we compare production perms with what we are pushing, and show the user:

image

@dwwoelfel @nezaj

console.log(diffedStr);

const okPush = await promptOk("OK to proceed?");
if (!okPush) return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a race condition in both push schema and perms, where if someone else pushed something in the meantime, we may potentially override it.

I think we can punt this race condition for now, but perhaps we can do in the future, is to pass in some version signifier, so in case it's not the same, we error out

Copy link

View Vercel preview at instant-www-js-rules-imp-jsv.vercel.app.

Copy link
Contributor

@dwwoelfel dwwoelfel left a comment

Choose a reason for hiding this comment

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

One comment, but LGTM! This will make pulling much nicer.


const diffedStr = jsonDiff.diffString(prodPerms.data.perms, perms)
if (!diffedStr.length) {
console.log("There are no changes to apply.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be obvious to the user that we're talking about perms changes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point! Will update the text 🫡

@stopachka stopachka merged commit fe13174 into main Nov 27, 2024
24 checks passed
@stopachka stopachka deleted the rules-imp branch November 27, 2024 00:39
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.

2 participants