Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Node.js deprecated install script #4407

Merged
merged 4 commits into from
Sep 8, 2023
Merged

Conversation

snaselj
Copy link
Contributor

@snaselj snaselj commented Sep 6, 2023

Closes: NaN

What's Changed

  • Fixed Dockerfile Node.js setup_XX.x deprecated script.
  • Split Dockerfile RUN into separate apt, npm and pip instructions for better readability.

Justification

There was 1 minute timeout during container build:

#12 14.37 ================================================================================
#12 14.37 ▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓
#12 14.37 ================================================================================
#12 14.37 
#12 14.37                            SCRIPT DEPRECATION WARNING                    
#12 14.37 
#12 14.37   
#12 14.37   This script, located at https://deb.nodesource.com/setup_X, used to
#12 14.37   install Node.js is deprecated now and will eventually be made inactive.
#12 14.37 
#12 14.37   Please visit the NodeSource distributions Github and follow the
#12 14.37   instructions to migrate your repo.
#12 14.37   https://github.com/nodesource/distributions
#12 14.37 
#12 14.37   The NodeSource Node.js Linux distributions GitHub repository contains
#12 14.37   information about which versions of Node.js and which Linux distributions
#12 14.37   are supported and how to install it.
#12 14.37   https://github.com/nodesource/distributions
#12 14.37 
#12 14.37 
#12 14.37                           SCRIPT DEPRECATION WARNING
#12 14.37 
#12 14.37 ================================================================================
#12 14.37 ▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓
#12 14.37 ================================================================================
#12 14.37 
#12 14.37 TO AVOID THIS WAIT MIGRATE THE SCRIPT
#12 14.37 Continuing in 60 seconds (press Ctrl-C to abort) ...
#12 14.37 
#12 74.37 
#12 74.37 ## Installing the NodeSource Node.js 18.x repo...

@snaselj snaselj changed the base branch from develop to next September 6, 2023 20:26
@snaselj snaselj closed this Sep 6, 2023
@snaselj snaselj reopened this Sep 6, 2023
@snaselj snaselj marked this pull request as ready for review September 6, 2023 21:01
@gsnider2195
Copy link
Contributor

Please rebase to develop

@bryanculver bryanculver changed the base branch from next to develop September 7, 2023 13:15
docker/Dockerfile Outdated Show resolved Hide resolved
apt-get install --no-install-recommends -y git mime-support libxml2 libmariadb3 openssl nodejs && \
apt-get autoremove -y && \
apt-get clean all && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line not required?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same question for the rest of the cleanup lines removed below

Copy link
Contributor

Choose a reason for hiding this comment

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

We need the rm -rf /tmp/tmp*. It tries to clean up that directory as the nautobot user later in the python-dependencies stage and fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this line not required?

It's not necessary, as apt temporary folders are mounted as cache, lines 58, 59

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same question for the rest of the cleanup lines removed below

The same, solved by mounting apt temporary folders to Docker cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the rm -rf /tmp/tmp*. It tries to clean up that directory as the nautobot user later in the python-dependencies stage and fails

/mnt is mounted as a Docker cache at line 77, so the line 78: RUN pip install ... instruction do not leave any files in /tmp on the image.

Copy link
Contributor

Choose a reason for hiding this comment

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

This build was failing for me when trying to delete a /tmp/tmp<random string> file yesterday but seems to be working now. Maybe I had a bad cache.

@snaselj
Copy link
Contributor Author

snaselj commented Sep 8, 2023

Merged with the latest develop to include venv removal.

@bryanculver bryanculver merged commit 139d59f into develop Sep 8, 2023
21 checks passed
@bryanculver bryanculver deleted the u/snaselj-docker-nodejs branch September 8, 2023 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants