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

DAS-1601 - Update platform for service Docker image to AMD64. #32

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

owenlittlejohns
Copy link
Member

Description

This PR is from a bit of a rabbit hole. I have made some changes to Harmony locally to prevent the ability to request PNGs or GIFs from HGA. To test that locally, I need the HGA image to work with Harmony in a Box - but I've changed machines since last running HiaB, and needed to update the --platform argument in the docker build command to make it all work.

Then for the rabbit hole - the Dockerfile was trying to pull in some things that had an issue, so I couldn't build the image without updating the source image that was being used. I tried a couple of approaches:

  • Migrating to the new ghcr.io hosted equivalent image. - This led to some compatibility issues when trying to install the Python bindings for GDAL.
  • Mimicking what has been done in HyBIG (using a Python image and using apt-get install libgdal-dev, but that also had a couple of issues).
  • So instead I've gone back to something that worked in MaskFill: using a Miniconda image and then using conda to install GDAL etc. Pretty unsatisfying, but it seems to be working.

All this, just to remove some functionality!

Bonus: I updated the change log to be consistent with our other projects. I haven't added URLs to the tags, because those tags don't all exist. (I might go through and manually make them a little later)

Jira Issue ID

DAS-1601 - Sort of. This PR enables testing of the Harmony core changes to switch off the ability to make requests that would run the code that should be removed in DAS-1601.

Local Test Steps

  • Pull this branch.
  • Run the tests locally: ./bin/build-image && ./bin/build-test && ./bin/run-test.
  • Also fire up HiaB and run the latest version of the HGA regression tests against this version of HGA.

PR Acceptance Checklist

  • Jira ticket acceptance criteria met. - Preliminary work.
  • version.txt and CHANGE.md updated if any service code is changed.
  • Tests added/updated and passing.
  • Documentation updated (if needed).

Copy link
Member

@flamingbear flamingbear left a comment

Choose a reason for hiding this comment

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

This looks good. I hate the conda but understand how you got there.

Two things. How did the tests run before on github actions? I don't see any runs in the actions.
I forked this trying to figure out how the actions worked and there are two warnings you might address.

https://github.com/flamingbear/harmony-gdal-adapter/actions/runs/11206162507

"The following actions use a deprecated Node.js version and will be forced to run on node20: actions/checkout@v3, docker/login-action@v2, docker/metadata-action@v4, docker/build-push-action@v3. For more info: https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/"

and:

Deprecation notice: v1, v2, and v3 of the artifact actions
The following artifacts were uploaded using a version of actions/upload-artifact that is scheduled for deprecation: "Coverage report for Python", "Test results for Python".
Please update your workflow to use v4 of the artifact actions.
Learn more: https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/

Since you're here you might update those actions.

bin/build-test Show resolved Hide resolved
.github/workflows/run_tests.yml Show resolved Hide resolved
Copy link
Member

@flamingbear flamingbear left a comment

Choose a reason for hiding this comment

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

This looks good. I hate the conda but understand how you got there.

Two things. How did the tests run before on github actions? I don't see any runs in the actions.
I forked this trying to figure out how the actions worked and there are two warnings you might address.

https://github.com/flamingbear/harmony-gdal-adapter/actions/runs/11206162507

"The following actions use a deprecated Node.js version and will be forced to run on node20: actions/checkout@v3, docker/login-action@v2, docker/metadata-action@v4, docker/build-push-action@v3. For more info: https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/"

and:

Deprecation notice: v1, v2, and v3 of the artifact actions
The following artifacts were uploaded using a version of actions/upload-artifact that is scheduled for deprecation: "Coverage report for Python", "Test results for Python".
Please update your workflow to use v4 of the artifact actions.
Learn more: https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/

Since you're here you might update those actions.

@owenlittlejohns
Copy link
Member Author

@flamingbear - good call on updating the stale versions of the GitHub actions. I've bumped those, and the things in run_tests.yml look to be working well now. I've also updated the Docker-owned actions. I checked out the documentation for each, and the syntax for the actions looks to be the same (login-action, metadata-action, build-push-action).

Copy link
Member

@flamingbear flamingbear left a comment

Choose a reason for hiding this comment

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

Looks good, is testing on my intel mac.

@owenlittlejohns owenlittlejohns merged commit 3a0b198 into main Oct 7, 2024
3 checks passed
@owenlittlejohns owenlittlejohns deleted the DAS-1601-preliminary branch October 7, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants