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

ci: get docker merge passing; fixes #283 #285

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

rockett-m
Copy link
Contributor

@rockett-m rockett-m commented Jul 18, 2024

Changed docker merge stage to run on pull-request in addition to merge to see if it will pass or fail before a merge takes place. .github/workflows/docker-push.yml
Although it is for testing, it might be valuable to keep this to avoid future issues

Edited the.dockerignore and Dockerfile so that scripts/activate-venv.sh is found
Error:

...
251.1 scripts/install-build-tools.sh: line 87: /scripts/activate-venv.sh: No such file or directory
-----------------------------------
Dockerfile:23
-----------------------------------
21 |      WORKDIR /opt/tx-processor
22 | 
23 |  >>> RUN scripts/install-build-tools.sh
24 |      RUN scripts/setup-dependencies.sh
----------------------------------
ERROR: failed to solve: process "/bin/sh -c scripts/install-build-tools.sh" did not complete successfully: exit code: 1
...

Added '.' to scripts/install-build-tools.sh sourcing the venv instead of 'source' due to Michael's point about sh not being able to run source.

@HalosGhost HalosGhost linked an issue Jul 18, 2024 that may be closed by this pull request
@rockett-m
Copy link
Contributor Author

@HalosGhost and/or @maurermi would you be able to review this? Thanks

Copy link
Collaborator

@maurermi maurermi left a comment

Choose a reason for hiding this comment

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

UT-ACK. This looks correct, will give it a test shortly, but I have high confidence. Thanks for patching this quickly.

.github/workflows/docker-merge.yml Outdated Show resolved Hide resolved
.dockerignore Outdated Show resolved Hide resolved
.github/workflows/docker-merge.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rockett-m rockett-m left a comment

Choose a reason for hiding this comment

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

feedback updated

I will remove the additional docker-merge run on pull-request when we are good on the changes pre-merge

docker-build.sh runs successfully on ubuntu 22.04 disk image with these changes fyi

Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
.dockerignore Outdated Show resolved Hide resolved
.github/workflows/docker-merge.yml Outdated Show resolved Hide resolved
@rockett-m rockett-m force-pushed the docker-merge branch 3 times, most recently from 2834d46 to 07c1262 Compare July 19, 2024 18:26
Signed-off-by: Morgan Rockett <morgan.rockett@tufts.edu>
@rockett-m
Copy link
Contributor Author

rockett-m commented Jul 19, 2024

Just ran and 8/8 checks passed. I had the docker merge stage included to see if it would pass.

I have now removed just the docker-merge on pull-request and re-pushed the commit. Should be good to go now

Copy link
Collaborator

@HalosGhost HalosGhost left a comment

Choose a reason for hiding this comment

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

in a seemingly related issue act is now broken testing against trunk, so I cannot reproduce this working locally; however, I saw the passing CI test, and I see that the push workflow finished correctly on your fork. I'll merge this in-hopes that it solves the immediate CI/CD problem, but a fix to generally narrow the gap between pull and merge docker workflows, ideally also fixing act for testing will be much appreciated.

@HalosGhost HalosGhost merged commit 0f7014f into mit-dci:trunk Jul 19, 2024
7 checks passed
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.

CI/CD breakage
3 participants