Skip to content

feat(publish): Store and restore publish state#192

Merged
BYK merged 8 commits into
masterfrom
byk/feat/publish-state
Mar 30, 2021
Merged

feat(publish): Store and restore publish state#192
BYK merged 8 commits into
masterfrom
byk/feat/publish-state

Conversation

@BYK

@BYK BYK commented Mar 29, 2021

Copy link
Copy Markdown
Member

A very common occurrence is publishing being interrupted in the
middle by some error, causing the whole publish operation to stop
and the operator to resume by explicitly determining the unpublished
targets and then passing these via the -t argument.

This patch adds a simple JSON state file, keyed by the release version
that gets deleted after a successful publish. When the file exists, it is
automatically used to skip over already published targets.

Fixes #179. Also refactors the publish command a bit to make things work
better with this new system.

A very common occurance is publishing being inturrupted in the
middle by some error, causing the whole publish operation to stop
and the operator to resume by explicitly determining the unpublished
targets and then passing these via the `-t` argument.

This patch adds a simple JSON state file, keyed by the release version
that gets deleted after a successful publish. When the file exists, it is
automatically used to skip over already published targets.
@BYK
BYK requested review from chadwhitacre, jan-auer and tonyo March 29, 2021 10:32
@BYK

BYK commented Mar 29, 2021

Copy link
Copy Markdown
Member Author

I think it is time to rethink how we handle SpecialTarget.All and SpecialTarget.None but this PR is already large so deferring that work.

Comment thread src/commands/publish.ts Outdated
Comment thread src/commands/publish.ts Outdated
Comment thread src/targets/cocoapods.ts

/** Options for "cocoapods" target */
export interface CocoapodsTargetOptions extends TargetConfig {
export interface CocoapodsTargetOptions {

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.

What's the main motivation behind this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No motivation, just pleasing TypeScript. Once I made the name field required, all these configs started to give type errors, complaining about a missing name attribute. Upon inspection, I realized none of these were actually extending TargetConfig, there was nothing shared. These were internal configs. What needed to be the TargetConfig is the one we pass in the constructor, which I refactored already.

Comment thread src/utils/files.ts Outdated

@chadwhitacre chadwhitacre left a comment

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.

Does this actually help us, though, in the context of a GitHub Actions deploy? How will the state file persist from one run to the next?

if (cleanupEnabled) {
// We intentionally do not block on the clean up operation
// so wait ~100ms before checking
await new Promise(resolve => setTimeout(resolve, 100));

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.

Sleep is never a good idea. I mean, sleep is a great idea ... but not here. No better way? Is this related to the await removals that @tonyo is asking about?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sleep is never a good idea. I mean, sleep is a great idea ... but not here.

Totally agree.

No better way?

We can try polling with a... timeout?

Is this related to the await removals that @tonyo is asking about?

Yup :(

@BYK

BYK commented Mar 29, 2021

Copy link
Copy Markdown
Member Author

Does this actually help us, though, in the context of a GitHub Actions deploy? How will the state file persist from one run to the next?

Not yet. We'll need to hook this up into our fail handler and then store/serialize this data to be unstored/unserialized at the start of the job.

@BYK
BYK requested review from chadwhitacre and tonyo March 29, 2021 19:23
@BYK
BYK merged commit c22598a into master Mar 30, 2021
@BYK
BYK deleted the byk/feat/publish-state branch March 30, 2021 16:11
BYK added a commit to getsentry/publish that referenced this pull request Apr 1, 2021
Builds on getsentry/craft#192 to limit targets to publish to. In a follow up, we'll add updating this list automatically on failure for idempotent retries on this repo.
BYK added a commit to getsentry/publish that referenced this pull request Apr 2, 2021
Builds on getsentry/craft#192 to limit targets to publish to. In a follow up, we'll add updating this list automatically on failure for idempotent retries on this repo.
BYK added a commit that referenced this pull request Apr 5, 2021
This is a follow up to #192. It fixes a bug that occurs when a
publish state file does not list all possible targets. In this case
any unlisted target, is skipped whereas its state should be treated
as "unpublished". This stemmed from a logic error in the if statement
which skipped publishing when the special target type "none" was passed.

The PR also refactors the code around this a bit to make it easier
to follow and less prone to errors. Ideal follow ups would be more
tests around publish and the state file functionality.
BYK added a commit that referenced this pull request Apr 6, 2021
This is a follow up to #192. It fixes a bug that occurs when a
publish state file does not list all possible targets. In this case
any unlisted target, is skipped whereas its state should be treated
as "unpublished". This stemmed from a logic error in the if statement
which skipped publishing when the special target type "none" was passed.

The PR also refactors the code around this a bit to make it easier
to follow and less prone to errors. Ideal follow ups would be more
tests around publish and the state file functionality.
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.

Make Craft more idempotent and add option to skip "completed" things

3 participants