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.

Sign up for GitHub

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

Jump to bottom

[ci] Make building debug images optional for PR CI #4663

Closed
yurishkuro opened this issue Aug 14, 2023 · 5 comments · Fixed by #4677
Closed

[ci] Make building debug images optional for PR CI #4663

yurishkuro opened this issue Aug 14, 2023 · 5 comments · Fixed by #4677
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

This all-in-one build took 15min. In it compiling Delve debugger, which is only used in debug images, took 6min.

Mon, 14 Aug 2023 20:33:53 GMT #17 [linux/arm64 build 3/3] RUN if [[ "arm64" == "s390x"  ||  "arm64" == "ppc64le" ]] ; then 	touch /go/bin/dlv;     else         go install github.com/go-delve/delve/cmd/dlv@latest;     fi
Mon, 14 Aug 2023 20:39:26 GMT #17 DONE 395.9s

I think we should skip it altogether when running the build for PR, let it only be built for CI runs on the main branch.
@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Aug 14, 2023
@yurishkuro
Copy link
Member Author

This was partially addressed by #4496 that is supposed to skip building debug images, but the logs above clearly show that's not enough. E.g. the script scripts/build-all-in-one-image.sh still calls make create-baseimg-debugimg where Delve compilation takes place, it needs to be changed too to respect the pr-only flag.

@karthikmurali60
Copy link
Contributor

karthikmurali60 commented Aug 15, 2023 •
edited
Loading

@yurishkuro I believe #4672 PR should resolve this issue.

@shubmjagtap shubmjagtap mentioned this issue Aug 15, 2023
Closed
@yurishkuro
Copy link
Member Author

@karthikmurali60 no, that PR only avoids building additional architectures, it does not change the fact that Delve is still being built

@karthikmurali60
Copy link
Contributor

@yurishkuro got it. The make create-baseimg-debugimg command is being called in two scripts -

  • build-upload-docker-images.sh
  • build-all-in-one-image.sh

Ideally in both of these places, we should execute the make command in only non PR mode right ??

@yurishkuro
Copy link
Member Author

We always need the main base image, but don't need debug image in pr-only

@Kavinjsir Kavinjsir mentioned this issue Aug 16, 2023
Merged
4 tasks
yurishkuro pushed a commit that referenced this issue Aug 16, 2023
@Kavinjsir
[ci] Avoid building debug image for PRs (#4677)
6b7f073 
## Which problem is this PR solving?
- Resolves #4663

## Description of the changes
- Update image build/upload scripts to create base image only when
`mode=pr-only`

## How was this change tested?
- Works from my local by running
`BRANCH=refactor/avoid-build-debug-img-in-pr
./scripts/build-all-in-one-image.sh pr-only`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

Signed-off-by: Tony Jin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[ci] Avoid building debug image for PRs Kavinjsir/jaeger
2 participants
@yurishkuro @karthikmurali60