Decouple telemetry API call from main action #24
Merged
Conversation
This file contains 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
TooManyBees
force-pushed
the
decouple-telemetry
branch
4 times, most recently
from
b688bb5 to
1058509
Compare
It gets cast to a string! Eep!
TooManyBees
force-pushed
the
decouple-telemetry
branch
from
1058509 to
a598b2a
Compare
TooManyBees
commented
Mar 29, 2022
| main() | ||
| } else { | ||
| // If emit_telemetry is not set, that indicates an older version of the dynamic workflow that doesn't separate telemetry from deployment | ||
| main().then(() => require('./pre')) |
There was a problem hiding this comment.
This is to accomodate the time between tagging this release as v1 and switching over to the new version of the dynamic workflow. We should expect to remove this last condition after both this action and the dynamic workflow on dotcom are deployed and in use.
There was a problem hiding this comment.
Just a note that when we revise this in that future PR, we should also either:
- set the default value of the
emit_telemetryinput to"false", and/or - reduce this section of the code to just use
else { ...rather thanelse if (emitTelemetry === "false").
TooManyBees
force-pushed
the
decouple-telemetry
branch
from
79abd3d to
32b8b86
Compare
YiMysty
reviewed
Mar 29, 2022
JamesMGreene
approved these changes
Mar 31, 2022
| main() | ||
| } else { | ||
| // If emit_telemetry is not set, that indicates an older version of the dynamic workflow that doesn't separate telemetry from deployment | ||
| main().then(() => require('./pre')) |
There was a problem hiding this comment.
Just a note that when we revise this in that future PR, we should also either:
- set the default value of the
emit_telemetryinput to"false", and/or - reduce this section of the code to just use
else { ...rather thanelse if (emitTelemetry === "false").
felipesu19
approved these changes
Mar 31, 2022
This has been an established pattern with Actions for a long time; we don't have to worry about it changing. Co-authored-by: James M. Greene <[email protected]>
Co-authored-by: James M. Greene <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
addresses https://github.com/github/pages-engineering/issues/1044
Changes deploy-pages to not automatically emit telemetry from the
src/pre.jsfile, both as a "pre" action and as a chained promise after the deploy completes. Instead, telemetry (from the same file) is only run when a new inputemit_telemetryis truthy.The matching PR to dotcom https://github.com/github/github/pull/213548 updates the dynamic workflow for jekyll page builds so that after the build, this action is invoked twice in parallel: once with
emit_telemetry: trueto make the telemetry API call, and once without to perform the deploy as usual (without any telemetry).People who use a "bring-your-own-workflow" to pages builds should not notice or care about this new input.
I expect to publish this version of the action to v1 before we deploy any code to dotcom that changes the dynamic workflow, so this action is designed to work for both current and newer versions of the dynamic workflow. When the
emit_telemetryinput is completely missing, this action falls back to its current behavior; when the input istrueorfalse, it adopts the new behavior that we want. Once the dotcom changes are deployed and we are confident in them, we can remove the fallback logic.