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

Migrate from Travis to GitHub Actions #1218

Merged
merged 21 commits into from
Aug 17, 2021
Merged

Migrate from Travis to GitHub Actions #1218

merged 21 commits into from
Aug 17, 2021

Conversation

colekettler
Copy link
Contributor

@colekettler colekettler commented Jul 21, 2021

Overview

Ports Travis CI config to a GitHub Actions workflow.

Checklist

  • Added needs-backport label if PR is bug fix that applies to previous minor release
  • Ran scripts/format_code and committed any changes
  • Documentation updated if needed
  • PR has a name that won't get you publicly shamed for vagueness

Testing Instructions

  • Verify that the GitHub Actions CI workflow passes.
  • Verify that this Release workflow ran.
  • Verify that the Release workflow has uploaded a working Docker image to Quay.io:
    docker pull quay.io/azavea/raster-vision:pytorch-test-gha
    
  • Run ./scripts/test locally and confirm that enabled tests run successfully and all pass.
    • Two semantic segmentation integration tests have been failing on master and have been disabled for now.
  • Verify that a code coverage report has been successfully uploaded to Codecov.
  • Confirm that Release workflow logic covers all versions previously pushed by .travis/deploy.

Closes #1129

@colekettler colekettler self-assigned this Jul 21, 2021
@colekettler colekettler force-pushed the cek/github-actions branch 2 times, most recently from 2227672 to 08eae33 Compare July 22, 2021 17:29
@colekettler
Copy link
Contributor Author

colekettler commented Jul 22, 2021

Codecov posts results in the PR. Fancy!

Comment on lines -5 to +7
rastervision/protos/**.py
rastervision/utils/filter_geojson.py
rastervision/utils/geojson.py
rastervision/utils/misc.py
rastervision/data/vector_source/label_maker/**.py
rastervision/data/label/tfod_utils/**.py
*/rastervision_core/rastervision/core/data/vector_source/label_maker/*.py
*/rastervision_core/rastervision/core/data/label/tfod_utils/*.py
*/rastervision_core/rastervision/core/utils/*.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the originally excluded file patterns no longer exist. I've tried to keep the current exclusions in the spirit of the original, ignoring vendored code and non-core utilities. I expect folks more familiar with which code paths are important can tweak this over time if we decide to keep up code coverage reporting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

runs-on: ubuntu-latest
strategy:
matrix:
image_type: [pytorch]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recognize that specifying a separate variant is likely a holdover from the transition from v1 to v2. However it's a pretty easy thing to interpolate in from the build matrix and it buys us a little extra flexibility later on. If this isn't actually useful I can hardcode it into the build and publish scripts instead.


- run: ./scripts/cibuild

- uses: codecov/codecov-action@v2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out Codecov publishes an action, so we don't have to script this ourselves anymore. It works well!

We also don't need to maintain an upload token since this is a public repo.

Comment on lines +7 to +9
- "[0-9]+" # Major version only
- "[0-9]+.[0-9]+" # Major and minor version
- "[0-9]+.[0-9]+.[0-9]+" # Major, minor, and patch version
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should cover release branches like 0.12, 0.12.1, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know what pattern matching engine is used here? May be more semantically correct to try https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall what the specific engine is, but I know it's not a PCRE engine and doesn't support capture groups or anything fancy like that: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet

Wildcard characters, digit ranges, and n-or-more matchers are about as far as it goes.

Comment on lines 23 to 24
QUAY_USER: ${{ secrets.QUAY_RASTERVISION_USER }}
QUAY_PASSWORD: ${{ secrets.QUAY_RASTERVISION_PASSWORD }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this secret to our organization and shared it with this repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +25 to +28
steps:
- uses: actions/checkout@v2

- run: ./scripts/cibuild

- uses: codecov/codecov-action@v2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is some duplication here with the CI workflow. It ended up being really tricky to filter and disambiguate semver release branches and PRs without getting into some more complicated property checks and regex magic. Splitting this out into a separate workflow felt clearer.

The way things are structured, PRs will only be built and have a test suite run on them with the CI workflow, and pushes to master / release branches / tags will have the same thing done plus pushing images to Quay.io. Everything only needs to get built and tested once per event. The separate workflows won't slow down overall build time or waste build minutes.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a reasoned tradeoff.

Comment on lines 32 to 34
- run: |
echo "GIT_BRANCH=${GITHUB_REF##*/}" >> $GITHUB_ENV
echo "GIT_COMMIT=${GITHUB_SHA:0:7}" >> $GITHUB_ENV
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the approach recommended by GitHub Actions to add environment variables. The GIT_BRANCH variable will snag the branch name or tag, and the GIT_COMMIT variable gets the short commit hash for image tagging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider allowing GIT_COMMIT to be sourced by the usual:

if [[ -n "${GIT_COMMIT}" ]]; then
    GIT_COMMIT="${GIT_COMMIT:0:7}"
else
    GIT_COMMIT="$(git rev-parse --short HEAD)"
fi

This works on GitHub Actions, since the Ubuntu runner has git installed. Also, consider determining the branch name in a similar fashion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The catch here is that I'd like for it to be available to the workflow itself, and then have it passed along to cipublish as the IMAGE_VERSION variable. That lets us condition what kind of tag we want to push in the workflow instead of needing to pass the ref into cipublish and then do branching logic in there to determine whether the tag should be the branch name, short commit hash, or latest.

We'd also need to decide if we want to push two tags in cipublish if we're on master (hash and latest), either in the script or by duplicating a check in the workflow config. That's kinda what I was trying to get away from in the old .travis/deploy script. Calling a simpler script twice with different arguments feels less complicated than a script that needs to have some nested conditional branches.

We could definitely do it that way, but I feel that keeping the branching logic closer to the Actions context it conditions on and having cipublish only ever do one thing, publish with the provided tag, is easier to read and understand. What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

The catch here is that I'd like for it to be available to the workflow itself, and then have it passed along to cipublish as the IMAGE_VERSION variable. That lets us condition what kind of tag we want to push in the workflow instead of needing to pass the ref into cipublish and then do branching logic in there to determine whether the tag should be the branch name, short commit hash, or latest.

This makes sense 👍 .

[...] That's kinda what I was trying to get away from in the old .travis/deploy script. Calling a simpler script twice with different arguments feels less complicated than a script that needs to have some nested conditional branches.

On the contrary, we've used conditional logic in cipublish for things like Scala artifact publishing.

We could definitely do it that way, but I feel that keeping the branching logic closer to the Actions context it conditions on and having cipublish only ever do one thing, publish with the provided tag, is easier to read and understand.

While I agree that having cipublish do only one thing is easier to read, I disagree that it is easier to understand. cipublish is supposed to wrap the workflow so that I could run cipublish on my laptop, and everything would work. Right now, it wouldn't work, and understanding the workflow requires you to look in two places.

What are your thoughts on this?

if [[ -n "${GITHUB_SHA}" ]]; then
    GITHUB_SHA="${GITHUB_SHA:0:7}"
else
    GITHUB_SHA="$(git rev-parse --short HEAD)"
fi

if [[ -n "${GITHUB_REF}" ]]; then
    GITHUB_REF="${GITHUB_REF}"
else
    # We don't know where GitHub sources the value of GITHUB_REF. git-describe
    # is close enough to be valid for publishing.
    GITHUB_REF="$(git describe --all)"
fi
docker login -u ${QUAY_USER} -p ${QUAY_PASSWORD} quay.io

if [[ "${GITHUB_REF}" == "refs/heads/master" ]]; then
    # On master, use the current SHA and latest
    docker tag "raster-vision-${IMAGE_TYPE}" \
        "quay.io/azavea/raster-vision:${IMAGE_TYPE}-${GITHUB_SHA}"
    docker push "quay.io/azavea/raster-vision:${IMAGE_TYPE}-${GITHUB_SHA}"

    docker tag "raster-vision-${IMAGE_TYPE}" \
        "quay.io/azavea/raster-vision:${IMAGE_TYPE}-latest"
    docker push "quay.io/azavea/raster-vision:${IMAGE_TYPE}-latest"
else
    # For everything else, use the branch or tag name
    docker tag "raster-vision-${IMAGE_TYPE}" \
        "quay.io/azavea/raster-vision:${IMAGE_TYPE}-${GITHUB_REF##*/}"
    docker push "quay.io/azavea/raster-vision:${IMAGE_TYPE}-${GITHUB_REF##*/}"
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's an excellent approach and I appreciate you taking the time to write it out so I can see how it looks! That completely addresses my concerns about too much nested conditional logic, reads well, and is more in line with our typical approach. I'll work this into cipublish.

Comment on lines 41 to 49
- run: ./scripts/cipublish
if: github.ref == 'refs/heads/master'
env:
IMAGE_VERSION: ${{ env.GIT_COMMIT }}

- run: ./scripts/cipublish
if: github.ref == 'refs/heads/master'
env:
IMAGE_VERSION: latest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're building a commit on master, we push an image tagged with both the short commit hash and latest.

Comment on lines 36 to 39
- run: ./scripts/cipublish
if: github.ref != 'refs/heads/master'
env:
IMAGE_VERSION: ${{ env.GIT_BRANCH }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're building a release branch or tag, we push an image tagged with that branch or tag version.

Dockerfile Outdated
ENV GDAL_DATA=/opt/conda/lib/python3.6/site-packages/rasterio/gdal_data/
ENV GDAL_DATA=/opt/conda/lib/python3.7/site-packages/rasterio/gdal_data/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were a few instances where we were still using 3.6, so I updated it to 3.7 across the board.

docs/release.rst Outdated
#. Using the Github UI, make a new release. Use ``0.8.1`` as the tag, and the ``0.8`` branch as the target.
#. The image for ``0.8`` is created automatically by Travis, but we need to manually create images for ``0.8.1``. For this you will need an account on Quay under the Azavea organization.
#. The image for ``0.8`` is created automatically by GitHub Actions, but we need to manually create images for ``0.8.1``. For this you will need an account on Quay under the Azavea organization.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this still true? It looks like we should automatically build and push minor version images just fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like this shouldn't be true anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll remove that bit from the documentation. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reading this more closely, it is still technically true. With how we currently tag releases, we will automatically release images for 0.8 based on the branch, and v0.8.1 based on our tags prefixed with v, but since there's no 0.8.1 branch or tag, that will still need to be done manually.

Is having both 0.8.1 and v0.8.1 actually valuable though? I do see both tags present on Quay.io, so we've been doing it, but I don't know if there was intent behind that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that having both 0.8.1 and v0.8.1 is valuable. If we are automatically releasing the bug fix versions based on tags that's good enough.

Comment on lines 48 to 62
'semantic_segmentation.basic': {
'task': 'semantic_segmentation',
'module': 'integration_tests.semantic_segmentation',
'kwargs': {
'nochip': False
}
},
'semantic_segmentation.nochip': {
'task': 'semantic_segmentation',
'module': 'integration_tests.semantic_segmentation',
'kwargs': {
'nochip': True
}
}
# Semantic segmentation tests currently failing, disabled until fixed
# 'semantic_segmentation.basic': {
# 'task': 'semantic_segmentation',
# 'module': 'integration_tests.semantic_segmentation',
# 'kwargs': {
# 'nochip': False
# }
# },
# 'semantic_segmentation.nochip': {
# 'task': 'semantic_segmentation',
# 'module': 'integration_tests.semantic_segmentation',
# 'kwargs': {
# 'nochip': True
# }
# }
Copy link
Contributor Author

@colekettler colekettler Jul 23, 2021

Choose a reason for hiding this comment

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

These tests are failing on master as well. I've disabled them for now and created #1220 to track fixing them when we are able to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw these got fixed in #1236 so I dropped the commit and rebased onto master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lewfish Heads up, I did run into a test failure here for semantic_segmentation.basic. Log lines in question are here.

Weirdly, it passed just fine on a subsequent run. Just wanted to raise that to your attention in case you run into flakey results later on.

Comment on lines 177 to 181
data=data, model=model, solver=solver, test_mode=test, log_tensorboard=True,
data=data,
model=model,
solver=solver,
test_mode=test,
log_tensorboard=True,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Formatting fix to satisfy the format checker.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in #1245.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @AdeelH! I'll drop this commit.

Comment on lines -3 to +4
coverage==4.5.1
codecov==2.0.15
coverage==5.5
codecov==2.1.11
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coverage fixed a few bugs since we last updated it, so an upgrade was necessary to get things working with Python 3.7 again.

Comment on lines +20 to +24
docker build \
--build-arg CUDA_VERSION="10.2" \
-t "raster-vision-${IMAGE_TYPE}" \
-f Dockerfile \
.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've generally removed conditional checks for whether we're building a pytorch image and interpolated in the IMAGE_TYPE instead. If we introduce other variant types later on and need to parameterize other things like the Cuda version, we can add it to the build matrix.

scripts/test Outdated
Comment on lines 20 to 23
if [[ -z ${CI} ]]; then
# Local test suite runs against pytorch image by default
IMAGE_TYPE=${IMAGE_TYPE:-pytorch}
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This script is run in CI to run the full test suite. It can also be used locally for sake of convenience, and will default to using the pytorch variant.

@colekettler colekettler marked this pull request as ready for review July 24, 2021 00:03
@colekettler colekettler requested a review from lewfish July 24, 2021 00:03
@colekettler
Copy link
Contributor Author

@lewfish Tagging you for review since you're familiar with the existing Docker image release workflow. I wanted to make sure I covered all the bases and that I'm not disrupting your local workflow!

Happy to discuss if you have questions about any of the GitHub Actions stuff. It replaces most Travis features pretty seamlessly but it is structured differently. I'll also look over the config with Rocky once he's available.

Copy link
Contributor

@lewfish lewfish left a comment

Choose a reason for hiding this comment

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

Nice job!

scripts/test Outdated
PYTORCH_IMAGE=raster-vision-pytorch

# Delete old coverage reports
docker run \
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a convention that scripts inside of docker/ run outside the docker container and invoke docker commands, and that scripts inside of scripts/ run inside the container. This breaks that convention. I don't really care, but just wanted to note this. Do other Azavea projects have a convention for this? Maybe the scripts and docker directories should all be merged into scripts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good callout, that's an important distinction.

Our typical convention is to merge them all into scripts and convert them to STRTA semantics. It's pretty common for scripts to run either on the host or inside of a Docker container depending on whether they're being run locally vs. on CI. docker/build would probably become scripts/update and docker/run would probably become scripts/server.

I'm hesitant to recommend that change without some further discussion though. The consistency that STRTA brings is incredibly useful, but its semantics were designed in the context of a web application. There's no server here so running scripts/server is kind of misleading, especially for an open source project that may have contributors outside of Azavea. It's also worth mentioning that STRTA was designed with Vagrant VMs in mind which were assumed to provide Docker and any necessary build dependencies without requiring any manual installation. That also isn't the case here.

This seems to be part of a larger thread of some of our web app conventions not quite fitting the bill for library and processing pipeline projects. I'll queue this up for discussion with Rocky.

In the short term I could add a check for whether the test script is running in a CI environment. If so, run tests in containers. If not, it's just an alias for the full test suite run outside of Docker on the host. We do this commonly and it would preserve existing conventions when run outside of CI. Does that sound good?

Copy link
Contributor

@lewfish lewfish Aug 2, 2021

Choose a reason for hiding this comment

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

In the short term I could add a check for whether the test script is running in a CI environment. If so, run tests in containers. If not, it's just an alias for the full test suite run outside of Docker on the host. We do this commonly and it would preserve existing conventions when run outside of CI. Does that sound good?

That sounds good.

I'm hesitant to recommend that change without some further discussion though. The consistency that STRTA brings is incredibly useful, but its semantics were designed in the context of a web application. There's no server here so running scripts/server is kind of misleading, especially for an open source project that may have contributors outside of Azavea. It's also worth mentioning that STRTA was designed with Vagrant VMs in mind which were assumed to provide Docker and any necessary build dependencies without requiring any manual installation. That also isn't the case here.

On second thought I would strongly prefer to not rename or move any of the existing scripts. We have a lot of other repos that use this one as a template and there's also a lot of muscle memory on the script names. Plus, like you say, the STRTA semantic don't apply so well on a repo like this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll get the test script updated and won't touch the Docker scripts. I've brought up STRTA semantics as they pertain to libraries with Rocky. That'll be an ongoing discussion between ops and library dev folks. We'll work out a common convention that makes sense for everybody.

Copy link
Contributor

@rbreslow rbreslow Aug 6, 2021

Choose a reason for hiding this comment

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

I stubbed out https://github.com/azavea/architecture/issues/10. I think we can both add context as we reflect @colekettler.

Comment on lines 48 to 62
'semantic_segmentation.basic': {
'task': 'semantic_segmentation',
'module': 'integration_tests.semantic_segmentation',
'kwargs': {
'nochip': False
}
},
'semantic_segmentation.nochip': {
'task': 'semantic_segmentation',
'module': 'integration_tests.semantic_segmentation',
'kwargs': {
'nochip': True
}
}
# Semantic segmentation tests currently failing, disabled until fixed
# 'semantic_segmentation.basic': {
# 'task': 'semantic_segmentation',
# 'module': 'integration_tests.semantic_segmentation',
# 'kwargs': {
# 'nochip': False
# }
# },
# 'semantic_segmentation.nochip': {
# 'task': 'semantic_segmentation',
# 'module': 'integration_tests.semantic_segmentation',
# 'kwargs': {
# 'nochip': True
# }
# }
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea.

Comment on lines -5 to +7
rastervision/protos/**.py
rastervision/utils/filter_geojson.py
rastervision/utils/geojson.py
rastervision/utils/misc.py
rastervision/data/vector_source/label_maker/**.py
rastervision/data/label/tfod_utils/**.py
*/rastervision_core/rastervision/core/data/vector_source/label_maker/*.py
*/rastervision_core/rastervision/core/data/label/tfod_utils/*.py
*/rastervision_core/rastervision/core/utils/*.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

docs/release.rst Outdated
#. Using the Github UI, make a new release. Use ``0.8.1`` as the tag, and the ``0.8`` branch as the target.
#. The image for ``0.8`` is created automatically by Travis, but we need to manually create images for ``0.8.1``. For this you will need an account on Quay under the Azavea organization.
#. The image for ``0.8`` is created automatically by GitHub Actions, but we need to manually create images for ``0.8.1``. For this you will need an account on Quay under the Azavea organization.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like this shouldn't be true anymore.

@lewfish
Copy link
Contributor

lewfish commented Aug 6, 2021

I fixed #1220 in #1236

Copy link
Contributor

@rbreslow rbreslow left a comment

Choose a reason for hiding this comment

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

Great work. Just a few comments on semantics to start.

Comment on lines +7 to +9
- "[0-9]+" # Major version only
- "[0-9]+.[0-9]+" # Major and minor version
- "[0-9]+.[0-9]+.[0-9]+" # Major, minor, and patch version
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know what pattern matching engine is used here? May be more semantically correct to try https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string.

Comment on lines 23 to 24
QUAY_USER: ${{ secrets.QUAY_RASTERVISION_USER }}
QUAY_PASSWORD: ${{ secrets.QUAY_RASTERVISION_PASSWORD }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +25 to +28
steps:
- uses: actions/checkout@v2

- run: ./scripts/cibuild

- uses: codecov/codecov-action@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a reasoned tradeoff.

Comment on lines 32 to 34
- run: |
echo "GIT_BRANCH=${GITHUB_REF##*/}" >> $GITHUB_ENV
echo "GIT_COMMIT=${GITHUB_SHA:0:7}" >> $GITHUB_ENV
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider allowing GIT_COMMIT to be sourced by the usual:

if [[ -n "${GIT_COMMIT}" ]]; then
    GIT_COMMIT="${GIT_COMMIT:0:7}"
else
    GIT_COMMIT="$(git rev-parse --short HEAD)"
fi

This works on GitHub Actions, since the Ubuntu runner has git installed. Also, consider determining the branch name in a similar fashion.

Comment on lines +20 to +24
docker build \
--build-arg CUDA_VERSION="10.2" \
-t "raster-vision-${IMAGE_TYPE}" \
-f Dockerfile \
.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider describing the image in a docker-compose.yml file?

Comment on lines +22 to +43
docker run \
--rm -t \
"raster-vision-${IMAGE_TYPE}" \
/opt/src/scripts/style_tests
docker run \
-w "$(pwd)" \
-v "$(pwd):$(pwd)" \
--rm -t \
"raster-vision-${IMAGE_TYPE}" \
/opt/src/scripts/unit_tests
docker run \
--rm -t \
"raster-vision-${IMAGE_TYPE}" \
/opt/src/scripts/integration_tests

# Create new coverage reports
docker run \
-w "$(pwd)" \
-v "$(pwd):$(pwd)" \
--rm -t \
"raster-vision-${IMAGE_TYPE}" \
coverage xml
Copy link
Contributor

@rbreslow rbreslow Aug 6, 2021

Choose a reason for hiding this comment

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

Ditto. I think this stuff would be more clear if we defined the working directories, volume mounts, TTY allocation, etc. in a docker-compose.yml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! I've thought about this a little more and I'd like to do this in a separate PR. We'd need to touch some other scripts like docker/run and docker/build if we want to avoid duplicating config, and we'd also need to introduce another development dependency (at least until Compose v2 is out of beta and is available with any recent version of Docker Engine). That's going to increase the diff a good bit and will require another more thorough round of local testing to make sure I'm not breaking anybody's workflow. That's something I'd like to make very visible as a separate decision from this migration.

I can get a follow-up issue set up prior to merging. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm fine with a follow up issue that outlines concerns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking in #1244.

scripts/cipublish Outdated Show resolved Hide resolved
@colekettler
Copy link
Contributor Author

@rbreslow I responded to your comments, coming back to you!

@colekettler colekettler force-pushed the cek/github-actions branch 2 times, most recently from 4f82da8 to cf3d0a8 Compare August 17, 2021 12:23
@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@b32e889). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1218   +/-   ##
=========================================
  Coverage          ?   57.97%           
=========================================
  Files             ?      168           
  Lines             ?     7636           
  Branches          ?        0           
=========================================
  Hits              ?     4427           
  Misses            ?     3209           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b32e889...f79d631. Read the comment docs.

@colekettler colekettler merged commit 4eaa709 into master Aug 17, 2021
@colekettler colekettler deleted the cek/github-actions branch August 17, 2021 14:59
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.

Migrate from Travis to GitHub Actions
4 participants