-
Notifications
You must be signed in to change notification settings - Fork 0
adding actions #2
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
Conversation
danschmidt5189
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see the test job fleshed out a bit before merging, especially since this involves changes to the Dockerfile (so we know the image will differ significantly from the current GitLab one).
| sftp = Net::SFTP.start( | ||
| 'ucopmft-in.ucop.edu', | ||
| 'cUCB100_library', | ||
| ENV['BFS_SFTP_USER'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like BFS has a centralized config module or other loading mechanism. In that case I think this is fine, but I want to call out that we should try to centralize config loading / definitions where possible, because it makes it much easier to see at a glance what options are available or necessary.
9302ca5 to
f1f831a
Compare
danschmidt5189
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to incorporate some multiplatform-build bug fixes from: https://github.com/BerkeleyLibrary/gha-testing/pull/3/changes#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34L3-R61
.github/workflows/release.yml
Outdated
| BASE_IMAGE: ${{ steps.get-base-image.outputs.tags }} | ||
| run: | | ||
| echo "$DOCKER_METADATA_OUTPUT_TAGS" | tr ' ' '\n' | xargs -n1 docker tag "$BASE_IMAGE" | ||
| docker push --all-tags "$(echo "$BASE_IMAGE" | cut -f1 -d:)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto for the recommendation above — this needs to be switched to the buildx method. See: https://github.com/BerkeleyLibrary/gha-testing/pull/3/changes#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R55
| @@ -1,18 +1,22 @@ | |||
| FROM registry.access.redhat.com/ubi8/ruby-31 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but let's be sure to switch this to the standard Ruby image and bump versions.
fixed deprecation issues with Docker removed reference to containers.lib image in docker-compose reset secrets in compose.ci moved COA user to environment removing tests for now. will need to be reworked removing tests for now. will need to be reworked updated read me and sftp user moving username to environement variable remove environment for tests, adding tests to build.yml renamed bfs service to app in docker compose Adding /opt/app directory and add artifacts to .gitignore referencing /opt/app as oppose to /opt/app-root/src as base directory adding file processing test removed path for image in container.lib skipping sftp for tests fixed typo for override in compose.ci syntax error in compose.ci build report was named Gobi instead of BFS using newer build and release template, slight syntax change in compose.ci reverting compose.ci to debug DOCKER_APP_IMAGE incorrectly mapped changed compose.ci to use array syntax
fixed deprecation issues with Docker
removed reference to containers.lib image in docker-compose
reset secrets in compose.ci
moved COA user to environment
removing tests for now. will need to be reworked
removing tests for now. will need to be reworked
updated read me and sftp user
moving username to environement variable