Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Update Dockerfile to use dotnet 7.0 #891

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

gsmachado
Copy link
Contributor

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
superboyiii previously approved these changes Feb 21, 2023
@superboyiii
Copy link
Member

@shargon Need your double check.

vncoelho
vncoelho previously approved these changes Feb 21, 2023
Jim8y
Jim8y previously approved these changes Feb 21, 2023
@gsmachado
Copy link
Contributor Author

gsmachado commented Feb 21, 2023

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
Copy link
Contributor

Jim8y commented Feb 22, 2023

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

@gsmachado
Copy link
Contributor Author

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

@gsmachado
Copy link
Contributor Author

gsmachado commented Feb 22, 2023

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
Copy link
Contributor Author

gsmachado commented Feb 22, 2023

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
Copy link
Member

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

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Maybe dotnet env is missing

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

@gsmachado
Copy link
Contributor Author

gsmachado commented Feb 22, 2023

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
Copy link
Contributor Author

gsmachado commented Feb 22, 2023

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

Attempt 1:

image

Attempt 2:

image

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

@gsmachado gsmachado dismissed stale reviews from Jim8y, vncoelho, and superboyiii via c1a4751 February 22, 2023 20:37
@gsmachado
Copy link
Contributor Author

gsmachado commented Feb 22, 2023

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
Copy link
Member

vncoelho commented Feb 22, 2023

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

@gsmachado
Copy link
Contributor Author

gsmachado commented Feb 22, 2023

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

@gsmachado
Copy link
Contributor Author

gsmachado commented Feb 22, 2023

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
Copy link
Member

Let me try on the browser later.

@gsmachado
Copy link
Contributor Author

Can someone merge?

@vncoelho vncoelho merged commit 8345b6e into neo-project:master Feb 23, 2023
@vncoelho
Copy link
Member

It was github mobile problem I was having.

@gsmachado
Copy link
Contributor Author

@vncoelho awesome, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants