-
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
base: main
Are you sure you want to change the base?
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.
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
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
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