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

Sync develop to master #270

Merged
merged 20 commits into from
Apr 24, 2024
Merged

Conversation

paulbourelly999
Copy link

PR Details

Description

This is an intermediate branch cut from master which includes fixes to the branch coupling of build and checkout scripts. This coupling can not yet be removed from CI processes because this repository still uses CircleCI. The functionality to decouple CI from branches is currently only available through our GitHub actions

Related GitHub Issue

Related Jira Key

Motivation and Context

Decouple build/checkout from branch to make release process simpler

How Has This Been Tested?

CI

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@MishkaMN
Copy link

Tried investigating the autoware failure again a bit more without success. Documented my progress here:
usdot-fhwa-stol/carma-platform#2369

@MishkaMN
Copy link

We know this is building at least on dockerhub so I think this is okay merging.
Moving on, we could have a short discussion with @JonSmet or @kjrush to accurately identify whether if these submodules are needed or not. Then we could perhaps even remove them to make the actions work.

MishkaMN
MishkaMN previously approved these changes Apr 23, 2024

FROM base_image AS build

ARG GIT_BRANCH=develop
Copy link

Choose a reason for hiding this comment

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

Since users will typically use the build-image.sh script directly, which calls this Dockerfile, do we want to update build-image.sh to take a git branch as an argument, and then pass it as a GIT_BRANCH arg into Dockerfile?

I suppose the same question can be asked for DOCKER_ORG and DOCKER_TAG as well.

Copy link

Choose a reason for hiding this comment

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

Or if this PR is only addressing the fix from an automated CI standpoint, we can create a separate story (or multiple) to fix it from a user standpoint.

Copy link
Author

Choose a reason for hiding this comment

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

I think they new arguments in the docker file will largely replace build-image.sh functionality that is responsible for setting org/tag for images.

Choose a reason for hiding this comment

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

I am okay with merging this since that build-images.sh script needed manual edits to work correctly anyways (such as checkout scripts or Dockerfile edits).
I do agree we can create new story to modify pass in these arguments based on what build-script.sh sets the metada.

.circleci/config.yml Outdated Show resolved Hide resolved
docker/checkout.bash Outdated Show resolved Hide resolved
@paulbourelly999
Copy link
Author

Fixed the failing docker build. Related to docker build context being default git context. Read https://docs.docker.com/reference/cli/docker/image/build/#git-repositories and https://github.com/docker/build-push-action?tab=readme-ov-file

MishkaMN
MishkaMN previously approved these changes Apr 23, 2024

FROM base_image AS build

ARG GIT_BRANCH=develop

Choose a reason for hiding this comment

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

I am okay with merging this since that build-images.sh script needed manual edits to work correctly anyways (such as checkout scripts or Dockerfile edits).
I do agree we can create new story to modify pass in these arguments based on what build-script.sh sets the metada.

JonSmet
JonSmet previously approved these changes Apr 23, 2024
MishkaMN
MishkaMN previously approved these changes Apr 23, 2024
@paulbourelly999 paulbourelly999 dismissed stale reviews from MishkaMN and JonSmet via 3a1e559 April 23, 2024 20:44
docker/checkout.bash Outdated Show resolved Hide resolved
@paulbourelly999 paulbourelly999 requested a review from JonSmet April 24, 2024 15:04
@paulbourelly999 paulbourelly999 merged commit ca90b2b into carma-develop Apr 24, 2024
1 of 2 checks passed
@paulbourelly999 paulbourelly999 deleted the sync-develop-to-master branch April 24, 2024 16:32
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.

4 participants