-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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: docker image tags documentation + tweaks #26923
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26923 +/- ##
=======================================
Coverage 67.19% 67.20%
=======================================
Files 1899 1899
Lines 74346 74346
Branches 8263 8263
=======================================
+ Hits 49960 49961 +1
+ Misses 22337 22336 -1
Partials 2049 2049
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
998584e
to
cead807
Compare
/testenv up |
@mistercrunch Ephemeral environment spinning up at http://34.220.34.38:8080. Credentials are |
Actions run their |
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.
Should docs have manual line breaks or should that just happen automatically? I pointed out an instance of that and linked to a recent comment from @john-bodley on this.
Otherwise a few suggestions for clarity but overall this looks great 👍
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.
docs LGTM
``` | ||
|
||
Presumably, `linux/arm64/v8` would be more optimized for this generation | ||
of chips, but less compatible across the ARM ecosystem. |
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.
This doc is super helpful. 🙌
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'm just not sure if it's 100% true ... seemed to work for me
scripts/build_docker.py
Outdated
@@ -205,12 +208,14 @@ def get_docker_command( | |||
) | |||
@click.option("--build_context_ref", help="a reference to the pr, release or branch") | |||
@click.option("--dry-run", is_flag=True, help="Run the command in dry-run mode.") | |||
@click.option("--force-latest", is_flag=True, help="Run the command in dry-run mode.") |
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.
Is the help string copy pasta?
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.
oopsy
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
4662ccf
to
a270ddb
Compare
Ephemeral environment shutdown and build artifacts deleted. |
Co-authored-by: Sam Firke <sfirke@users.noreply.github.com>
@supersetbot orglabel |
Co-authored-by: Sam Firke <sfirke@users.noreply.github.com>
This PR adds a new page to the docs under "Installation and Configuration" titled "Docker Images and Tags"
It also:
PR_5287
+ unit tests for thatlatest
) since the dev build isn't as secure--force-latest
flag to the build_docker.py scriptFor the text, I have GPT proof read it and I rendered it locally, and took a screenshot