Skip to content

build(craft): Update target execution order #4171

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

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

iker-barriocanal
Copy link
Contributor

@iker-barriocanal iker-barriocanal commented Nov 18, 2021

The goal of this PR is to minimize harm when a release fails. For example, last
week this release failed, and
with the changes this PR makes, the npm target wouldn't have run.

The order of targets Craft runs in a release matters. If a target fails, Craft
exits and stops the release, not running the targets that come after the failed
one. For this reason, targets that are easy to undo and don't have big
consequences should come first, and those that are harder to undo and have more
impact should run last; i.e. targets are sorted by ascending impact/cost of
reverting ratio.

Based on that guideline, this is the new order:

  1. aws-lambda-layer
    a. Users using new layers have to manually choose the version, so there's no
    risk of someone upgrading the version without manually doing it. See
    integration docs.
    b. It's easy to undo: logging in with a write-permission account and deleting
    the latest layer.
    c. Users aren't notified.
    d. The versions the AWS Lambda layer integration show are picked from the
    registry.
    e. Low impact, easy to revert.

  2. gcs
    a. Only affects CDN, and users have to manually upgrade the version. See
    CDN docs.
    b. Users aren't notified.
    c. Can revert it by removing the files from GCS.
    d. Low impact, easy to revert.

  3. github
    a. It may notify users.
    b. Creates a release, a tag, and links them on GitHub. To undo, can manually
    remove everything created from the GitHub UI.
    c. Medium impact, easy to revert.

  4. npm
    a. Affects users installing the SDKs for the first time, and to those upgrading
    them.
    b. High impact, can't revert after 72h.

  5. registry
    a. Sentry notifies customers based on updates to the registry. We don't want
    to notify them if a release doesn't go well.
    b. We own it and can revert the changes with ease, but the prior point is bad
    enough.
    c. Very high impact, can revert.

The order of targets Craft runs in a release matters. If a target fails, Craft
exits and stops the release, not running the targets that come after the failed
one. For this reason, targets that are easy to undo and don't have big
consequences should come first, and those that are harder to undo and have more
impact should run last; i.e. targets are sorted by ascending impact/cost of
reverting ratio.

Based on that guideline, this is the new order:

1. `aws-lambda-layer`
a. Users using new layers have to manually choose the version, so there's no
risk of someone upgrading the version without manually doing it. See
https://docs.sentry.io/platforms/node/guides/aws-lambda/layer/
b. It's easy to undo: logging in with a write-permission account and deleting
the latest layer.
c. Users aren't notified.
d. The versions the AWS Lambda layer integration show are picked from the
registry.
e. Low impact, easy to revert.

2. `gcs`
a. Only affects CDN, and users have to manually upgrade the version. See
https://docs.sentry.io/platforms/javascript/install/cdn/
b. Users aren't notified.
c. Can revert it by removing the files from GCS.
d. Low impact, easy to revert.

3. `github`
a. It may notify users.
b. Creates a release, a tag, and links them on GitHub. To undo, can manually
remove everything created from the GitHub UI.
c. Medium impact, easy to revert.

4. `npm`
a. Affects users installing the SDKs for the first time, and to those upgrading
them.
b. High impact, can't revert.

5. `registry`
a. Sentry notifies customers based on updates to the registry. We don't want
to notify them if a release doesn't go well.
b. We own it and can revert the changes with ease, but the prior point is bad
enough.
c. Very high impact, can revert.
@iker-barriocanal iker-barriocanal requested review from rhcarvalho and a team November 18, 2021 13:55
@iker-barriocanal iker-barriocanal self-assigned this Nov 18, 2021
@iker-barriocanal iker-barriocanal requested review from AbhiPrasad and removed request for a team November 18, 2021 13:55
@github-actions
Copy link
Contributor

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 22.49 KB (+0.02% 🔺)
@sentry/browser - Webpack 23.37 KB (0%)
@sentry/react - Webpack 23.4 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 29.93 KB (+0.01% 🔺)

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Do we need to change this in other SDKs as well?

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Verify good explanation of the motivation, thanks for writing it down.


  1. github
    ...
    b. Creates a release, a tag, and links them on GitHub. To undo, can manually
    remove everything created from the GitHub UI.

While it is technically possible to do, this can also have bad consequences. It is bad practice to delete or otherwise change published tags, and could break downstream systems (like caches/mirrors, forks/clones, CIs, ...).
Once published, we should never remove a tag/release from GitHub lightly.

@rhcarvalho
Copy link
Contributor

Do we need to change this in other SDKs as well?

Or the alternative -- make Craft run targets in a certain order regardless of the YAML config.

Pros: no need to change every repo's .craft.yml config.
Cons: removes flexibility from each repo to choose the order (though explicitly order could become an opt-in option).

@rhcarvalho rhcarvalho requested a review from mitsuhiko November 18, 2021 14:56
@iker-barriocanal iker-barriocanal merged commit a72ea44 into master Nov 19, 2021
@iker-barriocanal iker-barriocanal deleted the iker/build/craft-target-order branch November 19, 2021 10:05
onurtemizkan pushed a commit that referenced this pull request Dec 19, 2021
The order of targets Craft runs in a release matters. If a target fails, Craft
exits and stops the release, not running the targets that come after the failed
one. For this reason, targets that are easy to undo and don't have big
consequences should come first, and those that are harder to undo and have more
impact should run last; i.e. targets are sorted by ascending impact/cost of
reverting ratio.

Based on that guideline, this is the new order:

1. `aws-lambda-layer`
a. Users using new layers have to manually choose the version, so there's no
risk of someone upgrading the version without manually doing it. See
https://docs.sentry.io/platforms/node/guides/aws-lambda/layer/
b. It's easy to undo: logging in with a write-permission account and deleting
the latest layer.
c. Users aren't notified.
d. The versions the AWS Lambda layer integration show are picked from the
registry.
e. Low impact, easy to revert.

2. `gcs`
a. Only affects CDN, and users have to manually upgrade the version. See
https://docs.sentry.io/platforms/javascript/install/cdn/
b. Users aren't notified.
c. Can revert it by removing the files from GCS.
d. Low impact, easy to revert.

3. `github`
a. It may notify users.
b. Creates a release, a tag, and links them on GitHub. To undo, can manually
remove everything created from the GitHub UI.
c. Medium impact, easy to revert.

4. `npm`
a. Affects users installing the SDKs for the first time, and to those upgrading
them.
b. High impact, can't revert after 72h.

5. `registry`
a. Sentry notifies customers based on updates to the registry. We don't want
to notify them if a release doesn't go well.
b. We own it and can revert the changes with ease, but the prior point is bad
enough.
c. Very high impact, can revert.
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.

3 participants