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

Use docker in GA [AP-1085] #1393

Merged
merged 15 commits into from
Dec 18, 2023
Merged

Use docker in GA [AP-1085] #1393

merged 15 commits into from
Dec 18, 2023

Conversation

woodfell
Copy link
Contributor

@woodfell woodfell commented Dec 18, 2023

Description

@swift-nav/devinfra

Make use of libsbp-build docker image in the "check generated artefacts" stage. This bring consistency to the artefacts generated in the CI check since the canonical way of generating libsbp is via the docker image. This PR doesn't actually make any changes but should resolve some of the issues which have been seen recently when generating libsbp locally versus in CI.

API compatibility

Does this change introduce a API compatibility risk?

N/A

API compatibility plan

If the above is "Yes", please detail the compatibility (or migration) plan:

N/A

JIRA Reference

https://swift-nav.atlassian.net/browse/AP-1085

@woodfell woodfell force-pushed the woodfell/use_docker_in_ga branch from 761af2f to 37f688e Compare December 18, 2023 01:33
Copy link
Contributor Author

@woodfell woodfell left a comment

Choose a reason for hiding this comment

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

Overall brings generation inline with what's produced when docker is used locally. Should resolve annoying issues of late with getting this CI stage to pass

spec/yaml/swiftnav/sbp/acquisition.yaml Outdated Show resolved Hide resolved
- uses: gradle/gradle-build-action@v2
with:
gradle-version: 7.1.1
git config --global --add safe.directory /__w/libsbp/libsbp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure this is ideal but required to resolve a permissions issue which I think is caused by different UIDs being used inside and outside the container

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine/I don't know a better solution here.

.github/workflows/generator.yaml Outdated Show resolved Hide resolved
@woodfell woodfell marked this pull request as ready for review December 18, 2023 07:59
@woodfell woodfell requested a review from a team as a code owner December 18, 2023 07:59
Copy link
Contributor

@pcrumley pcrumley left a comment

Choose a reason for hiding this comment

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

This seems reasonable but I guess I'm curious why you abandoned goal of using latest and switched to a tagged version. I guess my concern is that we could run into this issue where the latest build gets broken if there is a delta between the two.

This situation is a lot better than it was, and very happy to see this work

@woodfell
Copy link
Contributor Author

This seems reasonable but I guess I'm curious why you abandoned goal of using latest and switched to a tagged version. I guess my concern is that we could run into this issue where the latest build gets broken if there is a delta between the two.

This situation is a lot better than it was, and very happy to see this work

It's so that we are able to build historical commit moving forwards. It we somehow bork the latest-master tag then older commits could be rendered unbuildable. Similarly, the situation we've just had is that some of the things which are pulled in to the image don't have their versions pinned and latest-master doesn't always generate exactly the same artefacts

@pcrumley
Copy link
Contributor

This seems reasonable but I guess I'm curious why you abandoned goal of using latest and switched to a tagged version. I guess my concern is that we could run into this issue where the latest build gets broken if there is a delta between the two.
This situation is a lot better than it was, and very happy to see this work

It's so that we are able to build historical commit moving forwards. It we somehow bork the latest-master tag then older commits could be rendered unbuildable. Similarly, the situation we've just had is that some of the things which are pulled in to the image don't have their versions pinned and latest-master doesn't always generate exactly the same artefacts

That seems very reasonable, maybe we should put the bumping of the build tag as part of the release process then

@woodfell woodfell changed the title Use docker in GA Use docker in GA [AP-1085] Dec 18, 2023
Copy link

Quality Gate Passed Quality Gate passed for 'libsbp-c'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@woodfell woodfell merged commit e473ea4 into master Dec 18, 2023
6 checks passed
@woodfell woodfell deleted the woodfell/use_docker_in_ga branch December 18, 2023 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.

2 participants