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

Run build_registry_docs as part of SDK build #1152

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

guineveresaenger
Copy link
Contributor

pulumi/pulumi-snowflake#752 revealed that we weren't running registry docs build on pull requests.
This addresses that issue. Index docs will now be built on pull requests in CI.

@guineveresaenger guineveresaenger requested review from a team and danielrbradley November 22, 2024 00:32
Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

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

I'm not sure this is the right place to put this command.

  1. It looks like we're running this on every parallel job in the build_sdk matrix, but it's doing exactly the same work in each branch.
  2. It doesn't look like we're doing anything with the result of the build - unless it's uploaded as part of each language's SDK, but my understanding from the build_registry_docs makefile target is it's just outputting the docs/ directory.
  3. If we're just trying to assert that the files are up to date, I think this needs to go before the "Check worktree clean" step.

Alternative options:

  1. Could we build this as part of the schema generation step? It's not exactly clear from the makefile what input files this target depends on running first.
  2. Perhaps we can model this dependency in the makefile rather than in CI so that it also runs automatically for anyone working on the provider locally?

Related to this .. I'm currently working on a larger piece of work to connect all the makefile targets together - so all dependencies are modelled even for local builds:

If it's a makefile change, I can make it as part of that PR instead.

@iwahbe
Copy link
Member

iwahbe commented Nov 22, 2024

I would implement this via the Makefile:

-build_sdks: build_nodejs build_python build_dotnet build_go build_java build_registry_docs
+-build_sdks: build_nodejs build_python build_dotnet build_go build_java
-tfgen: install_plugins upstream tfgen_no_deps
+tfgen: install_plugins upstream tfgen_no_deps build_registry_docs\

It feels like generating docs is more in the make tfgen category (with the schema) then as an SDK artifact.

@guineveresaenger
Copy link
Contributor Author

Unfortunately @iwahbe's suggestion won't work - make registry_docs depends on make tfgen (having the provider binary, with edit rules).

It doesn't look like we're doing anything with the result of the build
That's right, it's a plain "is this still valid" check

If we're just trying to assert that the files are up to date
Not quite - my approach was perhaps a bit simplistic - I want to run this as a separate check that would fail if there were docs changes incompatible with docs replaces that have a hard fail baked into them.

It sounds like two things should happen here:

  1. Perhaps run this as part of prerequisites so we don't run it four times 😅
  2. show the dependency on make tfgen in the Makefile.

@iwahbe
Copy link
Member

iwahbe commented Nov 25, 2024

Unfortunately @iwahbe's suggestion won't work - make registry_docs depends on make tfgen (having the provider binary, with edit rules).

This works for me (tested in random from a cleaned repo):

 Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 455c4723..225d1d7a 100644
--- a/Makefile
+++ b/Makefile
@@ -31,7 +31,7 @@ development: install_plugins provider build_sdks install_sdks
 
 build: install_plugins provider build_sdks install_sdks
 
-build_sdks: build_nodejs build_python build_dotnet build_go build_java build_registry_docs
+build_sdks: build_nodejs build_python build_dotnet build_go build_java
 
 install_go_sdk:
 
@@ -99,7 +99,7 @@ build_python: upstream
 		../venv/bin/python -m build .
 
 # Run the bridge's registry-docs command to generated the content of the installation docs/ folder at provider repo root
-build_registry_docs:
+build_registry_docs: upstream tfgen_build_only
 	$(WORKING_DIR)/bin/$(TFGEN) registry-docs --out $(WORKING_DIR)/docs
 
 clean:
@@ -155,7 +155,7 @@ test_provider:
 	@echo ""
 	cd provider && go test -v -short ./... -parallel $(TESTPARALLELISM)
 
-tfgen: install_plugins upstream tfgen_no_deps
+tfgen: install_plugins upstream tfgen_no_deps build_registry_docs
 
 tfgen_no_deps: export PULUMI_HOME := $(WORKING_DIR)/.pulumi
 tfgen_no_deps: export PATH := $(WORKING_DIR)/.pulumi/bin:$(PATH)

It incurs minimal overhead of make tfgen (only the actual docs generation is a new target) and builds correctly from a clean repo.

@guineveresaenger
Copy link
Contributor Author

I'd been holding off on this due to #1151 - with those changes, we have build_registry_docs as a separate Make target dependent on make schema(tfgen) and we can run it as-is in the prerequisites workflow.

@guineveresaenger guineveresaenger added this pull request to the merge queue Nov 26, 2024
Merged via the queue into master with commit eabcbac Nov 26, 2024
6 checks passed
@guineveresaenger guineveresaenger deleted the guin/build-registry-docs branch November 26, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants