Skip to content
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

chore: Use released code for publishing docs #3754

Closed
wants to merge 1 commit into from

Conversation

spalladino
Copy link
Collaborator

Removes the need to set INCLUDE_RELEASED_CODE when building docs, and just builds off the published images of sandbox and l1-contracts on dockerhub. This ensures that the API reference we are generating is actually from the latest release and not from whatever's on master (which is a bug we have today).

Note that we keep the INCLUDE_RELEASED_CODE logic in our docs build script just to try it out locally.

Removes the need to set INCLUDE_RELEASED_CODE when building docs, and
just builds off the published images of sandbox and l1-contracts on
dockerhub.
Comment on lines +1223 to +1225
filters:
branches:
only: master
Copy link
Collaborator Author

@spalladino spalladino Dec 20, 2023

Choose a reason for hiding this comment

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

Happy to change this for a branch check in each step of the job for consistency with deploy-and-release

@spalladino
Copy link
Collaborator Author

Note that this doesn't work as-is since include-code macros need access to both released code and latest code (as a fallback). This only gives access to latest code.

spalladino added a commit that referenced this pull request Dec 21, 2023
This is a quick and dirty fix to get docs to load released code snippets
again. We should not need to pass the entire git repo to the build
context to make it work, but it should buy us some time until we ship a
proper solution (see #3754).
Maddiaa0 pushed a commit that referenced this pull request Jan 8, 2024
This is a quick and dirty fix to get docs to load released code snippets
again. We should not need to pass the entire git repo to the build
context to make it work, but it should buy us some time until we ship a
proper solution (see #3754).
@spalladino
Copy link
Collaborator Author

Comments from Charlie on this PR from back in the day, so they don't just sit on my Slack:

We generate docs from l1-contracts as well? Sad face. (edit: ah, I see it's all around messaging stuff)
Docker ignore files can be next to their Dockerfile if named same as Dockerfile and postfixed .dockignore e.g. Dockerfile.prod.dockerignore
Why do we need a second Dockerfile and job/manifest entries? Do we not just make docs always build from released version? i.e. why not replace Dockerfile with Dockerfile.prod, what do we need Dockerfile for now?
buildDir can now be docs, which means in theory you can just name the dockerignore file .dockerignore.
No rebuild patterns required now, as we only need rebuild if docs changes, which is the default pattern if buildDir is docs and no rebuildPatterns are provided. (edit: Hmmmmm. This is true in the master case, but not in the tagged case, as the code may have changed, but the docs may not have. But it's also unnecessary to rebuild the docs if master code changed, as we don't use it in the docs. Perhaps the "correct" solution, is to use release-please to update the version tag on the FROM , this means its always clear what it's building off, and that the content-hash changes. Technically we could inject the version into any file covered by the rebuildPatterns, but that might be the most sensible thing.)
Shouldn't the job that depends on deploy-and-release deploy-docs, be the one that runs only for tags? and then we want to run the same job, but with no dependencies, on master. In fact for simplicity (otherwise we actually need to duplicate the same job), can we just have the same job run for both master and tags? Means that a master release has to wait for the whole pipeline but think that's ok for simplicity.
Also we're kind of using the term "release" for publishing stuff, and "deploy" for running infrastructure. So maybe name job release-docs.

And also an issue with this approach:

I spotted an issue with the solution I pushed: new docs that reference code from unreleased files need to be able to read from master. [...] We can handle this by building FROM the dockerhub released version and FROM the ecr cache, and copying them to different folders. It's doable, we just need to tweak this script a bit.

michaelelliot pushed a commit to Swoir/noir_rs that referenced this pull request Feb 28, 2024
…3761)

This is a quick and dirty fix to get docs to load released code snippets
again. We should not need to pass the entire git repo to the build
context to make it work, but it should buy us some time until we ship a
proper solution (see AztecProtocol#3754).
@spalladino
Copy link
Collaborator Author

Closing in favor of #4928

@spalladino spalladino closed this Mar 4, 2024
@ludamad ludamad deleted the palla/load-docs-code-from-dockerhub branch August 22, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant