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

Added verify-book-links make target #3194

Merged
merged 1 commit into from Feb 15, 2022

Conversation

zeborg
Copy link
Contributor

@zeborg zeborg commented Feb 11, 2022

Signed-off-by: Abhinav Sinha abhinav@nirmata.com

What type of PR is this?
/kind feature

What this PR does / why we need it:

  • Added verify-book-links make target
  • Upgraded image maven:3-jdk-14 to maven:3-openjdk-17-slim in hack/tools/plantuml.Dockerfile for multi-arch (amd64 and arm64) support
  • Updated docs/proposal/20200506-single-controller-multitenancy-flow.svg to the newly generated one

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3176

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

Added `verify-book-links` make target

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Feb 11, 2022
@k8s-ci-robot
Copy link
Contributor

@zeborg: This issue is currently awaiting triage.

If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 11, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @zeborg. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 11, 2022
@zeborg zeborg force-pushed the verify-book-links branch 2 times, most recently from ea17d45 to f4b1c07 Compare February 11, 2022 02:47
@sedefsavas
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 11, 2022
Makefile Outdated
@@ -615,6 +615,11 @@ verify-conversions: $(CONVERSION_VERIFIER) ## Verifies expected API conversion
verify-shellcheck: ## Verify shell files
./hack/verify-shellcheck.sh

.PHONY: verify-book-links
verify-book-links: ## Verify book links
echo verification of book links initiated
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo verification of book links initiated

@zeborg
Copy link
Contributor Author

zeborg commented Feb 11, 2022

Hi @sedefsavas, looking at the failed logs it seems like in order for this test to succeed we also need to install Docker in the verify-book-links CI job. I'd like to know if I should implement this change as well. Thanks!

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 11, 2022
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 11, 2022
@zeborg
Copy link
Contributor Author

zeborg commented Feb 11, 2022

Update:

Hi @sedefsavas, I tried to check if Docker was present in the CI environment, and it seems like it is already installed but the docker daemon is not running. I'm not sure about what the underlying OS is, and if the env privileges would allow us to start the docker daemon through shell. I might need your input here to understand how I should further proceed with this issue.
Thanks!

For context, here are the logs from the failed run:

499 make -C docs/book verify
500 make[1]: Entering directory '/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/docs/book'
501 make -C ../ diagrams
502 make[2]: Entering directory '/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/docs'
503 make -C ../hack/tools bin/plantuml-sentinal
504 make[3]: Entering directory '/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/hack/tools'
505 docker build --build-arg PLANTUML_VERSION=1.2020.16  . -f plantuml.Dockerfile -t "plantuml-builder"
506 touch bin/plantuml-sentinal
507 failed to dial gRPC: cannot connect to the Docker daemon. Is 'docker daemon' running on this host?: dial unix /var/run/docker.sock: connect: no such file or directory
508 make[3]: *** [Makefile:161: bin/plantuml-sentinal] Error 1
509 make[3]: Leaving directory '/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/hack/tools'
510 make[2]: *** [../common.mk:50: ../hack/tools/bin/plantuml-sentinal] Error 2
511 make[2]: Leaving directory '/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/docs'
512 make[1]: *** [Makefile:61: generate] Error 2
513 make[1]: Leaving directory '/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/docs/book'
514 make: *** [Makefile:620: verify-book-links] Error 2

@richardcase
Copy link
Member

Our verify implementation in different to upstream capi (they just call mdbook build).

Was this working ok previously? Probably not.

Actually i think i can see the project....

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2022
@richardcase
Copy link
Member

@sedefsavas @zeborg - as we want to run a container as part of the verify make target we need to run the prow job in privileged mode. This should do that:

kubernetes/test-infra#25240

@sedefsavas
Copy link
Contributor

Our verify implementation in different to upstream capi (they just call mdbook build).

I think we can add that too. We only test uml creation as is.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2022
@zeborg zeborg force-pushed the verify-book-links branch 2 times, most recently from cfd77e8 to f81969d Compare February 15, 2022 05:29
@richardcase
Copy link
Member

I just made a change to the prow job definition so that we execute the make target via runner.sh which setups DIND:

kubernetes/test-infra#25264

Signed-off-by: Abhinav Sinha <abhinav@nirmata.com>

Added echo statement

Signed-off-by: Abhinav Sinha <abhinav@nirmata.com>

Upgraded image in `plantuml.Dockerfile` to `maven:3-openjdk-17-slim`

Signed-off-by: Abhinav Sinha <abhinav@nirmata.com>

Made suggested changes

Signed-off-by: Abhinav Sinha <abhinav@nirmata.com>
@richardcase
Copy link
Member

/test pull-cluster-api-provider-aws-verify

@richardcase
Copy link
Member

Doesn't look like Prow has the updated job definition yet.....lets try again in a bit.

@richardcase
Copy link
Member

/test pull-cluster-api-provider-aws-verify

@richardcase
Copy link
Member

Looking more promising this time:

 Docker in Docker enabled, initializing...
================================================================================
Starting Docker: docker.
Waiting for docker to be ready, sleeping for 1 seconds. 

Fingers crossed

@richardcase
Copy link
Member

Yay it passed

@richardcase
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 15, 2022
@k8s-ci-robot k8s-ci-robot merged commit 370dbc9 into kubernetes-sigs:main Feb 15, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.x milestone Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add verify-book-links to Makefile target
5 participants