-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
docs: Improve builds for Envoy and mobile (stamping) #29887
Conversation
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
c875218
to
adaf91b
Compare
75d2cdc
to
b30a712
Compare
ae01e80
to
65ab4f6
Compare
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
d69b054
to
8cc3d03
Compare
/docs |
Docs for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-pr/29887/docs/index.html The docs are (re-)rendered each time the CI |
8543a17
to
801229e
Compare
Signed-off-by: Ryan Northey <[email protected]>
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, overall LGTM, minor comments/suggestions.
@@ -246,11 +246,15 @@ pkg_tar( | |||
genrule( | |||
name = "html_release", | |||
outs = ["html_release.tar.gz"], | |||
# BUILD_SHA must be set in release builds |
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.
Will/can the script fail if it is not set?
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.
yeah it will - bazel enforces set env vars so it fails
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.
(ditto re too many vars)
# The full version, including alpha/beta/rc tags. | ||
release = os.environ['ENVOY_DOCS_VERSION_STRING'] | ||
release = version |
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.
Seems there's a mismatch between the description (comment) of release
and version
described 4 lines above, though both should be the same.
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.
there are too many vars here - in this PR im doing the minimum to get things working but happy to remove some of this superflous code in a follow up
fwiw - in a follow up - this will no longer happen here anyway i think - at least in relation to publishing (rather than testing)
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.
hmm actually realizing this is the code that will be triggered still - so this wont be moved - but it is my intention to clean this up
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.
SGTM.
Next time I suggest adding a TODO, but no need for it in this PR.
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.
LGTM, thanks!
# The full version, including alpha/beta/rc tags. | ||
release = os.environ['ENVOY_DOCS_VERSION_STRING'] | ||
release = version |
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.
SGTM.
Next time I suggest adding a TODO, but no need for it in this PR.
Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: phlax <[email protected]>
Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: phlax <[email protected]>
Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: phlax <[email protected]>
Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: phlax <[email protected]>
This removes
docs/build.sh
and limits the role ofmobile/docs/build.sh
In the case of the first its now only used for presubmit testing, and for mobile it will be the same once we enable the new mobile docs site.
currently these scripts are required to set up the env for builds - this should happen in the genrule and use workspace stamping where appropriate. Removing them will make it easier to build both sets of docs as external targets without the need for these scripts or to simulate what they are doing
This should also make docs building much faster as it doesnt use
action_env
to set the docs config and expire all caches on every runCommit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]