Potential path towards addressing #49

[hellkite500]

I separated the build and push action after assuming that some contention existed in apply the push component on each PR. This new action and workflow will only attempt to build (but not publish/push) the ngen dockerfile when either the Dockerfile.ngen or Dockerfile.ngen-deps is changed in a PR. This may need a little additonal testing/refactoring, but I'm trying to understand what the base need is and how to get some validation of proposed changes running on relevant PRs to check if the changes fix and/or break the build used in the rest of the CD pipeline.

Does this start getting to that problem?

[hellkite500]

This could replace #52 if it better meets the intended use/need.

[benlee0423]

Closing this PR because the refactor #74 resolves the issue of build on PR.

[hellkite500]

It seems to me that #74 enables the entire build AND push pipeline for every PR (and subsequently, for any new commit added to the PR). This could be quite problematic for several reasons -- particularly pushing a new tagged image for each PR when the features haven't yet been reviewed and merged opens the door to a lot of possible bad outcomes, not to mention extra compute and effort. I would strongly recommend reviewing this PR's proposed changes and thinking about having two distinct workflows, one to test builds on PR but nothing else, and another (the existing workflow) to build and push on merge to main.

[arpita0911patel]

I agree, we should think about having two seperate pipeline one to test PRs and another one for merge to main. @benlee0423 can you please take alook. Also #74 we have to currently manually update the variable in Settings with the tag name to use, which requires manual changes. We need to find better way to tag the image for PRs.

[benlee0423]

@hellkite500
Can you fix your change? I don't seem to find a way to run your workflow. You might need it build in your personal repo and put the result here.
https://github.com/CIROH-UA/NGIAB-CloudInfra/actions/runs/7424953937

[benlee0423]

This does not work because just detecting Dockerfile.ngen or Dockerfile.ngen-deps, or Dockerfile in PR does not detect upstream changes in ngen and t-route repo. The new image is not the same as previous image when there is no changes in Dockerfiles. We need to build and test it every PR.
A question to be answered is do we want to build and test new image before merge to main? What if build was successful, but new image does not work?

[benlee0423]

every PR (and subsequently, for any new commit added to the PR). This could be quite problematic for several reasons -- particularly pushing a new tagged image for each PR when the features haven't yet been reviewed and merged opens the door to a lot of possible bad outcomes, not to mention extra compute and effort.

Somewhat agree if we want to merge to main without testing new image.
Somewhat disagree based on our PR frequency. We are not making a lot of changes daily basis. At most one or two PRs a week, it should be fine in this low PR frequency. When we create a PR, we need to review, build, and test your own pull request before submitting it. This will allow anyone to catch errors or typos that he/she may have missed, before others start reviewing. It involves a couple of commits to be added on the PR based on the review. If one's code is not ready to create PR, we can still make draft PR first, and work on the draft without making a PR that doesn't trigger any actions.
Here is best practices on PR.
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/best-practices-for-pull-requests

[benlee0423]

currently manually update the variable in Settings with the tag name to use, which requires manual changes. We need to find better way to tag the image for PRs.

Fixed in #76

[hellkite500]

Can you fix your change?

The failure in the existing workflow isn't caused by the proposed changes here. The issue is with the dockerhub login in the current action steps here.

Run echo "" | docker login --username awiciroh --password-stdin
  echo "" | docker login --username awiciroh --password-stdin
  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
Error: Cannot perform an interactive login from a non TTY device
Error: Process completed with exit code 1.

detecting Dockerfile.ngen or Dockerfile.ngen-deps, or Dockerfile in PR does not detect upstream changes in ngen and t-route repo

The intent of the proposed workflow here is to help with the development and maintenance of these dockerfiles! If a PR changes one of the dockerfiles, then we need to minimally know that the dockerfile is still valid and that the image builds with those changes. This could be just one set of steps in a more complex pipeline, but are intended to give early and automatic feedback on direct changes to the IaC components in the dockerfiles, and doing so in a way that tries to reduce the amount of resource needed in the process.

The next logical step would be to use the results of this workflow in a cache (the built image is cached upon success) and then the next pipline/workflow could do a functional test of the binary built within the image.

When we create a PR, we need to review, build, and test your own pull request before submitting it. This will allow anyone to catch errors or typos that he/she may have missed, before others start reviewing

I agree this is a generally good practice, but isn't always tractable, especially given environment differences, hardware differences, ect...we have already seen a few instances of hard to reproduce errors and issues that come up even using docker (arm vs x86 architecture and emulation issues ect...). The more places and options we have to test and validate, the more of these types of things we may be able to uncover before something gets released! The trick is designing the tests and the workflows to be efficient and provide the feedback required for the developer(s) proposing changes and using as much early stopping as possible.

does not detect upstream changes in ngen and t-route repo

If this is the goal, we definitely need some additional, and separate, tests and pipelines and this was not the intent of this particular PR. A successful build and environment is one required step, a working binary inside the container (environment) is another, and we can definitely work to design and implement solutions with that goal.

[arpita0911patel]

Thank you Nels, I agree. Lets see if #86 resolves this issue.

[arpita0911patel]

Closing this PR as the idea here has been incorporated in the Forked repo CI pipeline that Ben is developing.