feat(docker): increase node version in docker to 12

[Klaitos]

New Pull Request Checklist

• I am not disclosing a vulnerability.
• I am creating this PR in reference to an issue.

Issue Description

Related issue: #1787

Approach

TODOs before merging

• Add tests
• Add entry to changelog
• Add changes to documentation (guides, repository pages, in-code descriptions)

[parse-github-assistant]

Thanks for opening this pull request!

• 🎉 We are excited about your hands-on contribution!

[dblythy]

Thank you @Klaitos for the investigation and quick PR!!

[dblythy]

I think this line will need to be updated as well:

parse-dashboard/package.json

Line 131 in 7686c70

"node": ">=8.9"

[cbaker6]

This one and the one below should probably use the node tag, lts-alpine instead of 12 directly.

[cbaker6]

It seems, 12 builds, but lts doesn't. It may be something in the npm packages as this occurs with lts: https://github.com/netreconlab/parse-dashboard/runs/3545759058?check_suite_focus=true#step:7:378

[mtrezza]

ci tests don't pass for node 16 because node-sass (also in the logs you linked) needs to be bumped it seems. maybe that's why? not sure which node version lts-alpine uses.

[cbaker6]

ci tests don't pass for node 16 because node-sass (also in the logs you linked) needs to be bumped it seems. maybe that's why?

Makes sense, should it be bumped in this PR?

[mtrezza]

I tried, it seems to requires some code refactoring because this is a chain reaction where we need to bump webpack to 5 as well. To make dashboard officially Node 16 compatible will be an undertaking of its own.

[cbaker6]

@mtrezza did something change in the docker CI after I added #1781? I don't see the docker CI testing this PR as it would of when I made my changes. I think it's important to make sure PR's don't break the docker image before they go to the master branch.

Update: I see your changes in #1786 removed the docker CI for PR's

[mtrezza]

Yes, the flow "docker-publish" is a publish flow that runs after merging a PR into master. If we want to check whether a PR breaks anything, we need to add a docker build check in the ci.yaml. I am adding this in #1789, and a couple of other things.

[cbaker6]

Yes, the flow "docker-publish" is a publish flow that runs after merging a PR into master. If we want to check whether a PR breaks anything, we need to add a docker build check to the CI.

It was doing this automatically in my original PR, when you removed:

  pull_request:
     branches: [ master ]

It stopped this PR check

[mtrezza]

Yes, but the reason for #1786 is that we need to stop updating the docker hub latest tag on every merge into master, if we want to transition the latest tag to be the "latest stable". We are in a transition now, as we'll implement the new branch model probably next week. So 3.0 release could be a good cut for how we use the latest label.

[cbaker6]

Yes, but the reason for #1786 is that we need to stop updating the docker hub latest tag on every merge into master, if we want to transition the latest tag to be the "latest stable".

It looks like it's updating latest because of the lines added 1786:

          flavor: |
             latest=${{ steps.branch.outputs.branch_name == 'master' }}

If those lines aren't there, it will update to latest stable on releases and tags. It's currently updating everytime a commit is made to the master which is what you are saying you don't want, check here: https://hub.docker.com/r/parseplatform/parse-dashboard/tags?page=1&ordering=last_updated

master has the same time stamp and digest as latest.

So currently, your latest is ahead of your last stable release because of the lines added above. Basically, my original PR 1781 was doing what you are asking, the updates in 1786 are not

[mtrezza]

The original PR pushes "latest" to docker hub on every commit, we don't want to do that for now. I thought on push only runs for direct push, not for PR merges, so I'd also remove the branches: [ master ] and the schedule.

Then we only have:

on:
  push:
    tags: [ '*.*.*' ]

That means, only when tags a pushed a docker image is released and it will probably have the version tag and latest tag. Once we have set up the new branch model, we need to redesign this all anyway, because we'll push beta and alpha tags to docker.

And to make sure a PR doesn't break the docker image, I'll add a CI step to ci.yaml, that is just trying to build the image.

What do you think?

[cbaker6]

The original PR pushes "latest" to docker hub on every commit, we don't want to do that for now. I thought on push only runs for direct push, not for PR merges, so I'd also remove the branches: [ master ] and the schedule.

My original PR does not do this. I showed you the example of my repo, it's not updating latest on every commit, nor does it update latest on nightly. The docker action handles this correctly, there's no need to add anything for flavor or removing the lines I put before. You can see this by looking at my tags. I've been adding updates see docker and nightly tags, and latest hasn't been updated for over 1 day https://hub.docker.com/repository/registry-1.docker.io/netreconlab/parse-dashboard/tags?page=1&ordering=last_updated

[mtrezza]

Yes, I meant the original one, before you removed the flavor. I thought we were not sure what the auto behavior for flavor is and when it will push to the latest tag? Because I'm not sure whether that will happen when we push a version tag 3.0.0 - will it create a docker image with tag 3.0.0 and another identical one with latest?

[mtrezza]

Everything below node 12 reached its EOL, so we should make Node 12 the minimum and also use node 12 for the docker image.

[mtrezza]

Looks fine!

Next steps are:

1. Merge, that triggers a docker push so we have both master and latest pointing to an image that works.
2. Adapt the docker-publish CI to be triggered only when a tag is pushed
3. Add CI step to ensure dependencies don't require a higher node engine than the host package

Step 3 should prevent this issue from happening again.

[Klaitos]

Let me know if i can help somehow

[mtrezza]

Maybe you could try out the new docker image to see whether it works fine.

[Klaitos]

will do !

[Klaitos]

➜  Desktop docker run  -p 8080:4040 -v "/parsedashboard.json:/src/Parse-Dashboard/parse-dashboard-config.json" parseplatform/parse-dashboard:latest       

The dashboard is now available at http://0.0.0.0:4040/

I can confirm the dashboard works on the latest tag👏

Thanks @mtrezza

[mtrezza]

Amazing! Thanks for investigating this issue to thoroughly!

[cbaker6]

I thought we were not sure what the auto behavior for flavor is and when it will push to the latest tag?

I don't know all of the logistics of auto, but I do know some... It doesn't update latest when merging into the master branch using the way my PR, 1781 was merged (note latest is currently being updated on every merge because of PR 1786).

Because I'm not sure whether that will happen when we push a version tag 3.0.0 - will it create a docker image with tag 3.0.0 and another identical one with latest?

This will create a docker image for 3.0.0 as well as update latest... Leaving the schedule as I had it in 1781 will produce a nightly tag and still not update latest.

[mtrezza]

Thanks for clarifying, that is good to know for when we remodel the branches. For now we'll only push release version tags to docker (which should then also update the latest to be the "latest stable") to have more control over the pushes while we work on the new branches; see #1789.