neo-project / neo-node

MIT License
229 stars 224 forks source link

Update Dockerfile to use dotnet 7.0 #891

Closed gsmachado closed 1 year ago

gsmachado commented 1 year ago

As mentioned here, I believe that once again the Dockerfile was simply ignored after the upgrade from dotnet 6 to 7.

It already happened more than once: developers change code but they simply forget to update other parts of the repo that do not fail on the build time. There are some Neo projects relying on this Dockerfile, especially for tests.

This PR makes the whole GitHub Action workflow fail if anything is merged and something with Dockerfile image is not right (e.g., wrong dotnet version, or anything like that).

The next step would be to enhance to check if the Docker image at least starts with the node.. but that's a bit more work and I haven't included it in this PR. One thing at a time. 😄

superboyiii commented 1 year ago

@shargon Need your double check.

gsmachado commented 1 year ago

The GitHub Action Docker Build Test is failing somehow. But it passed on my branch.

Can someone check what's wrong? I think it's something related to dotnet...

image
Jim8y commented 1 year ago

funny, it passed test on my branch as well https://github.com/Liaojinghui/neo-node/actions/runs/4243191118/jobs/7375617322

gsmachado commented 1 year ago

Can someone restart this GitHub Action? 🙏 I don't have permission for doing it.

gsmachado commented 1 year ago

Absolutely strange.

My branch (from where I'm performing the PR): ✅ https://github.com/AxLabs/neo-node/actions/runs/4227623939/jobs/7383281773

It passes.

gsmachado commented 1 year ago

It complains about a Name property that is not set.

Someone with more dotnet knowledge can check this, please?!?! 🙏

image

Check the build error here: https://github.com/neo-project/neo-node/actions/runs/4227689982/jobs/7382971225

vncoelho commented 1 year ago

Let's do in two PR,@gsmachado . First, upgrade. Then, the new feature.

gsmachado commented 1 year ago

Maybe dotnet env is missing

- name: Setup .NET Core uses:
 actions/setup-dotnet@v1 
with: dotnet-version: ${{ env.DOTNET_VERSION }}

No, this is unrelated, 100% sure. No need for dotnet version within the GitHub Action environment since the docker image (see the file here) contains the correct dotnet version and tooling coming from Microsoft (as a docker image).

gsmachado commented 1 year ago

The most strange thing: there were 2 attempts. Both failed with DIFFERENT errors.

Attempt 1:

Attempt 2:

However, BOTH attempts failed when building a docker image for linux/arm64. Coincidence?

gsmachado commented 1 year ago

Alright! Fixed. 👍

@vncoelho, @Liaojinghui, @shargon, @superboyiii: can any of you approve & merge? 😸

From now on, if devs commit code and docker image doesn't build, the whole CI/CD will (intentionally) miserably fail. 👀

vncoelho commented 1 year ago

Maybe just add the new task at the end of the current file,@gsmachado

gsmachado commented 1 year ago

@vncoelho try to go to GitHub on your desktop/browser and merge. And not on your mobile phone. 😄

gsmachado commented 1 year ago

Maybe just add the new task at the end of the current file,@gsmachado

I'd rather prefer to keep things separate, as they should be. 😸

Do you see the same error on the browser (desktop) version of GitHub?

vncoelho commented 1 year ago

Let me try on the browser later.

gsmachado commented 1 year ago

Can someone merge?

vncoelho commented 1 year ago

It was github mobile problem I was having.

gsmachado commented 1 year ago

@vncoelho awesome, thanks!