-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat(build): pass git commit sha for ReSpec based specs #167
Conversation
21f7941
to
043dade
Compare
Useful if https://github.com/w3c/respec/pull/4561 gets merged
043dade
to
74980a5
Compare
src/prepare-build.ts
Outdated
@@ -26,16 +32,20 @@ export async function buildOptions(inputs: Inputs) { | |||
const flags = []; | |||
flags.push(...getFailOnFlags(toolchain, inputs.BUILD_FAIL_ON)); | |||
|
|||
return { toolchain, source, destination, flags, configOverride }; | |||
return { toolchain, source, destination, gitRevision, flags, configOverride }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think best would be add the gitRevision
to each config override above, instead of setting it later.
Does Bikeshed set it automatically in build process, or we can pass it via config also? If not passable, can pass conditionally for ReSpec only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bikeshed does it automatically already on its own
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you say a bit more why you think it should be in the override? this feels typically like a setting that shouldn't need to be overridden by a static configuration file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was suggesting that only because as we can to pass it down less manually that way, as configOverride
will get passed to ReSpec as respecConfig
override (via URL parameters).
https://github.com/w3c/spec-prod/pull/167/files#diff-f065c431b4f45758b2e047fcd38135fdcb2327564def7a33ab41e84898210fc8L95-L101
We use same method for setting publishDate
:
spec-prod/src/prepare-build.ts
Lines 139 to 140 in 958b6d5
if (toolchain === "respec") { | |
conf.publishDate = publishDate = conf.publishDate || publishDate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'll refactor this part someday, I find it confusing too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you be willing to make the code work like you think it should? I'm struggling a bit…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you can add following above and reset src/build.ts
, that'd be enough:
const configOverride = {
gh: getConfigOverride(inputs.GH_PAGES_BUILD_OVERRIDE),
w3c: getConfigOverride(inputs.W3C_BUILD_OVERRIDE),
};
+ if (toolchain === 'respec) {
+ configOverride.gh.gitRevistion = configOverride.w3c.gitRevision = gitRevision;
+ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not, feel free to do it what you find simpler. We can refactor it eventually.
Based on discussion in #167 (review)
Based on discussion in #167 (review)
7507bce
to
25b82dd
Compare
Based on discussion in #167 (review)
25b82dd
to
9c08a98
Compare
Based on discussion in #167 (review)
9c08a98
to
333cdd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dontcallmedom!
I hope to be properly back at maintaining these projects soon!
Useful if https://github.com/w3c/respec/pull/4561 gets merged