This repository was archived by the owner on Sep 30, 2024. It is now read-only.
Merged
Conversation
LawnGnome
commented
Jul 16, 2021
| className, | ||
| }) => ( | ||
| <div className={classNames(iconClassNames, className)} data-tooltip="Set published: true to publish to code host"> | ||
| <div className={classNames(iconClassNames, className)}> |
Contributor
Author
There was a problem hiding this comment.
We could try to propagate the changeset spec down to figure out what message to show, but since this was already kind of inaccurate (drafts exist!), I think I'd rather deal with this in documentation.
Contributor
|
Notifying subscribers in CODENOTIFY files for diff 3329cdf...ba410f8.
|
mrnugget
approved these changes
Jul 16, 2021
mrnugget
reviewed
Jul 16, 2021
eseliger
approved these changes
Jul 16, 2021
Member
eseliger
left a comment
There was a problem hiding this comment.
LGTM 🌟 Thanks for changing this so quickly after yesterdays discussions!
We should get back to Thorstens comment as a follow-up but we needed to do that anyways and it's not worse than before so I don't have any problem merging this.
Co-authored-by: Thorsten Ball <[email protected]>
…ngesetsModal.tsx Co-authored-by: Thorsten Ball <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is part seven of #18277. Many thanks to @eseliger for pointing me at some prior art, and everyone else on the team for signing on to this plan.
I've reused the hedged "attempt to X" wording from the close changesets bulk action: I know we didn't all love it, but it's more technically correct than saying we're definitely going to publish. I've also made this fairly forgiving: you can try to publish things that you can't publish, and you only get bulk operation errors rather than hard failures when you try to execute the bulk operation in the first place.
At one point, I had this implemented with a glorious single query. (Seriously, look at that. It's beautiful. I totally ripped off another query in this file, but I think I made it my own.) Unfortunately, doing so kind of breaks the assumptions of bulk operations: I could easily fake a changeset job with the IDs in the payload and have it operate, but the UI would be wrong when it showed the number of affected changesets. I toyed with adding a concept of a chunked changeset job, or making changeset jobs have a many-to-many relationship with changesets, but ultimately it felt like I was paddling upstream, so I reverted back to a simpler approach mimicking the other bulk actions.
Practically, this doesn't have much effect: there's a bit more scope for conflicting operations, but nothing should break even if the UI and spec publication states are changed repeatedly and in conflict. (I tried.) Mostly, it'll just make things a bit slower, and it means that updating a scheduled changeset will put it to the back of the queue. (Which is a hell of a corner case: you'd have to have a UI draft changeset and then change it to UI published while it was scheduled, but it's technically valid.)