This should be build docker image -> ./gradlew dockerBuildImages

I think, we are missing downloading deps, right?

populate_and_save_cache:
    description: 'Downloads all gradle dependencies and uploads cache for later use'
    steps:
      - gradle:
          args: downloadDependencies
      - save_cache:
          paths:
            - ~/.gradle
          key: v1-dependencies-{{ checksum "/tmp/checksum.txt" }}

This should be for all the branches including forks.

This will only trigger whenever we cut the release from their UI. How about if someone pushes the tag directly.

can we have a fixed version for this action docker-practice/actions-setup-docker@master ?

Here as well, we should use the fixed version for OS

What is the value of this cache key?

We should not run dockerPushImages in context of pull_request_target.

done

Can we make this job as build-and-test? And move both unit-test and integration steps as part of this job only?

reverted to this

We will need pull_request_target here, right?

Curious about the difference between master-build and pr-build?
As pr-build is also getting run on trigger - on:push, right?

The only difference I see here is that dockerPushImages. can we have if condition in pr-build that it happens only for merged to main. So, we don't need this entire workflow which is the same. Does this make sense?

That is how it was earlier

reverting it back to earlier state. We discussed having separate merge-publish so this was done.

Also needs validate-helm-charts right?

yup

I assume this is the target rather than source branch - any reason to restrict it to main? Running on any pull request seems sufficient.

Further, even though it's unnecessary, is there really any significant benefit to excluding markdown files as triggers? It only takes (or at least should) a few minutes to run, and the added complexity of having filter rules doesn't seem worth it to me. Particularly because this would mean we can no longer require status checks on protected branches, as a md-only change wouldn't meet that requirement.

Okay. I agree. This will be required check so it doesn't make sense to have it here.

This is on any branch or tag creation - we don't need that, do we?

Does this replace pull_request or is it in addition to? if it's in addition to, it seems like it should have the same filter rules

I removed filters from both. And yes, it's in addition to it. So pull_request will fail on forks but pull_request_target will pass

This has already been run, right? For publish, we're not re-testing - we're assuming the code is valid to have made it to this point.

yup

missing restore keys

Can this execute bash code? I assume this needs to be a constant value since we're outside the run context

We can easily avoid putting secrets in the env here - is it any safer? If there were a script in the below run block, the secret would be compromised. Can we inline them instead to lower the possibly exposures (assuming GH doesn't expose them via the commands being run)?

Changed this because of https://docs.github.com/en/free-pro-team@latest/actions/reference/encrypted-secrets#using-encrypted-secrets-in-a-workflow .
here they mentioned

Avoid passing secrets between processes from the command line, whenever possible. Command-line processes may be visible to other users (using the ps command) or captured by security audit events. To help protect secrets, consider using environment variables, STDIN, or other mechanisms supported by the target process.

This is interesting to me - you're right, they say to do it this way, and they're right, that traditionally you don't want to pass sensitive data via CLI. The part that I'm caught on though, is that this is running in an isolated container (so we don't have to worry about other processes or users seeing via ps, at least I think?), but on the other hand, environment variables are available to everything executing in this step so if a script were executing, it could be modified to read the environment variable.

I'm certainly open to be convinced, but my suspicion is that those docs aren't considering the full picture here. If we can't find any other info on it, my recommendation would be to use the STDIN approach and pipe the secrets (for example, our old docker login command was echo $DOCKERHUB_PASSWORD | docker login --username $DOCKERHUB_USERNAME --password-stdin) - that way neither concern applies.

@JBAhire - you're right, because this is not an external workflow, the secret protection can be a little looser - this is fine.

See pr build comments about triggers

See pr build comments about triggers

Should we be providing pull credentials for dockerhub images to avoid rate limiting, or does GH have some work around for that?

yeah

nit: assuming github.event.pull_request.merged is already a boolean, the == true part isn't needed.

validate charts should run in pull_request_target right?

missing creds like in validate-helm-charts

Will this work if forked PR merged to main?

yup. It will work. pull_request_target also has a same webhook pull_request.

ref: https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#pull_request_target

Can you move this form env to with as this runs as part of pull_request_target?

Nope. Doesn't work in that way.

Do we have to login here?

Will remove this in separate PR as we want to test it and owner's approval is require.

The snyk and pr-build workflow are having the same condition. Can we move this job as part of pr-build.yml workflow?

As an example, below is the current scenario.

May less workflow makes it easy to look for

1. pr-build (build-and-test)
2. publish (release)

The validate-charts and pr-build workflow are having the same trigger condition. Can we move this job as part of pr-build.yml workflow?