Build containers only once and also do it for forks

[sharkwouter]

This PR changes the following:

• Always build the containers only once (at the moment they are build twice for commits to master)
• If this is the master branch, push containers to the repository's container registry first. This makes it so forks can also get container images.

What stays the same:

• If this is the master branch and the XboxDev repo specifically, also push containers to the group's registry with the tags which we're currently used to (nxdk:git-..., nxdk-debug:latest, etc).
• If this is the master branch and the XboxDev repo specifically, also push containers to the docker hub with the tags which we're currently used to (nxdk:git-..., nxdk-debug:latest, etc).

This is a first step towards a multi-step build using docker. I'd like to use that to be able to build packages which can be installed using a package manager I've been working on. I think this specific part is useful regardless of if that's the way you guys will want to go in the end, though.

[JayFoxRox]

I don't like these new names.

debug and lto are also latest, aren't they?

[RadWolfie]

nxdk/.github/workflows/build_docker_image.yml

Lines 61 to 67 in a82c2c0

	if [ "${{ matrix.tag }}" == "latest" ]; then
	TARGET="ghcr.io/${{ github.repository_owner }}/nxdk"
	else
	TARGET="ghcr.io/${{ github.repository_owner }}/nxdk-${{ matrix.tag }}"
	fi
	docker tag ghcr.io/${{ github.repository }}:${{ matrix.tag }} ${TARGET}:latest
	docker tag ghcr.io/${{ github.repository }}:${{ matrix.tag }} ${TARGET}:git-${GITHUB_SHA::8}

@JayFoxRox, according to new changes, if latest tag is selected, then it will exclude suffix tag of the "tag" from matrix processing. Yet I agree with you for the nxdk tag replacement is confusing.

latest tag should be change to release tag to follow up the confusion JayFoxRox brought up for clarification. Since it is compiling a release build. Correct?

[sharkwouter]

So the tags and container names in the dockerhub and xboxdev group are not different with this PR compared to without this PR. Sorry if this looks a bit confusing.

The reason for these tags in the registry of the repo itself is that for docker containers the tag latest is the default tag, which docker will automatically use if no tag is provided. Alternative builds tend to have their own unversioned tag names or different container names (which is not possible here, since we can only use nxdk as container name in this case). The way I'm tagging here is in line with standard practises on the Docker Hub for example, so I think not using latest would cause more confusion.

Hopefully that clarifies why I picked these tag names. Let me know if you still want me to change it, then I can still do so.

[RadWolfie]

Hmm... According the doc https://docs.docker.com/engine/reference/commandline/tag/#tag-an-image-referenced-by-name, tag isn't what you think it is. It's more or less like a branch name or in this case image name. So, using matrix's tag as an image name isn't accurate reference.

Other flaw I see from this PR changes is it push nxdk, the image, tag with (via latest, debug, lto) as a commit tag. In order word, it messes with nxdk package as placeholder storage for the next matrix to able use it. And it will affect if multiple pushed/merged simultaneously which each will have different delay to start the process. I'm fairly sure this isn't the way we want it to do. Plus cause more confusion with nxdk's "nxdk" package's tags vs other packages which are currently in private I suspect. Using GH's artifact, cache, or other alternative are recommended to do this if we want to handle the docker image within GH's Action temporary before push to package registery as permanent storage.

With that said, I don't see the reason for need to remove GH actions? They're there to simplified the process. And unnecessary to do login every step. We can perform if check for each step to skip them.

[sharkwouter]

I see what you mean, I think I addressed this now unless I misunderstood. The latest commit removes the need to pull in between, resolving the race condition you mentioned.

[RadWolfie]

This line is affected mentioned by https://github.com/XboxDev/nxdk/pull/656/files#r1278720076.

Other flaw I see from this PR changes is it push nxdk, the image, tag with (via latest, debug, lto) as a commit tag. In order word, it messes with nxdk package as placeholder storage for the next matrix to able use it. And it will affect if multiple pushed/merged simultaneously which each will have different delay to start the process. I'm fairly sure this isn't the way we want it to do. Plus cause more confusion with nxdk's "nxdk" package's tags vs other packages which are currently in private I suspect. Using GH's artifact, cache, or other alternative are recommended to do this if we want to handle the docker image within GH's Action temporary before push to package registery as permanent storage.

[sharkwouter]

I made this a bit cleaner now. Hope it helps.

[thrimbor]

I've read through all the changes as carefully as I could and they look good to me. I do think the commits should get consolidated before we merge though to get rid of fix-up commits. Squashing it all into a single commit would be fine imho.

[sharkwouter]

Thanks for the review @thrimbor! I've squished the commits into a single one now. Can you take another look? I've not made any other changes.