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

Tp1 239 cleanup package.json #12903

Merged
merged 13 commits into from
Oct 17, 2024
Merged

Tp1 239 cleanup package.json #12903

merged 13 commits into from
Oct 17, 2024

Conversation

dlopezvsr
Copy link
Collaborator

@dlopezvsr dlopezvsr commented Sep 18, 2024

Description

This PR cleans up the package.json npm scripts, removing redunant scripts already created on tasks.py, and also deleting duplicates. Some scripts where also migrated from package.json and to tasks.py in order to keep consistency on the scripts and leave as simple as possible the npm scripts section. Only scripts necessary to build the application were left, and also the main ones that are called from tasks.py.

Link to sample test page: https://foundation-s-tp1-239-cl-2nnvbk.herokuapp.com
Related PRs/issues: #11767

Checklist

Tests

  • Is the code I'm adding covered by tests?

Changes in Models:

  • Did I update or add new fake data?
  • Did I squash my migration?
  • Are my changes backward-compatible. If not, did I schedule a deploy with the rest of the team?

Documentation:

  • Is my code documented?
  • Did I update the READMEs or wagtail documentation?

Merge Method
💡❗Remember to use squash merge when merging non-feature branches into main

┆Issue is synchronized with this Jira Story

@dlopezvsr dlopezvsr self-assigned this Sep 18, 2024
@dlopezvsr dlopezvsr marked this pull request as ready for review September 19, 2024 20:10
@dlopezvsr dlopezvsr temporarily deployed to foundation-s-tp1-239-cl-2nnvbk September 19, 2024 20:10 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-tp1-239-cl-aqtacc September 19, 2024 20:12 Inactive
@dlopezvsr dlopezvsr temporarily deployed to foundation-s-tp1-239-cl-zfnatt September 23, 2024 19:18 Inactive
@dlopezvsr dlopezvsr temporarily deployed to foundation-s-tp1-239-cl-giy8te September 23, 2024 19:43 Inactive
Copy link
Collaborator

@danielfmiranda danielfmiranda left a comment

Choose a reason for hiding this comment

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

Thanks for taking on this work @dlopezvsr!

The PR looks good to me, going to tag @mmmavis to take a look at package/npm changes as they are a little more familiar 👍

@dlopezvsr
Copy link
Collaborator Author

@danielfmiranda thank you for your review. I'm also looking forward to @mmmavis comments. 🙌

@dlopezvsr dlopezvsr temporarily deployed to foundation-s-tp1-239-cl-4dlnia September 25, 2024 19:19 Inactive
@mmmavis
Copy link
Collaborator

mmmavis commented Oct 4, 2024

Sorry for the wait 😅 . I will give this PR a look tomorrow!

Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

Hey @dlopezvsr , thank you for diving into package.json and Docker to make things more streamlined!

Apologies for leaving tons of comments. I'm not a Docker expert, so the best I could do was by testing each command. Unfortunately, I was getting errors for the majority of new inv commands introduced in this PR. A few were due to run-s and run-p, and some were due to "commands not found" in the Docker environments. I think those packages probably need to be installed globally or somehow configured for Docker to be properly run as inv command. Or we can leave them in package.json for simplicity.

Let me know if I can help with anything. Maybe we can explore the Docker wonderland together to find solutions to these issues lol

tasks.py Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
@dlopezvsr
Copy link
Collaborator Author

dlopezvsr commented Oct 7, 2024

Hey @dlopezvsr , thank you for diving into package.json and Docker to make things more streamlined!

Apologies for leaving tons of comments. I'm not a Docker expert, so the best I could do was by testing each command. Unfortunately, I was getting errors for the majority of new inv commands introduced in this PR. A few were due to run-s and run-p, and some were due to "commands not found" in the Docker environments. I think those packages probably need to be installed globally or somehow configured for Docker to be properly run as inv command. Or we can leave them in package.json for simplicity.

Let me know if I can help with anything. Maybe we can explore the Docker wonderland together to find solutions to these issues lol

@mmmavis thank you so much for taking this deep look at the changes and for all the comments, I will review each one and apply the necessary fixes. I remember to test most of them and run them without error, but I also made some changes later with some commands that I put back into the package.json due to some dependency errors in the build, so I may have missed removing them form inv commands.

@dlopezvsr
Copy link
Collaborator Author

@mmmavis thank you so much again for all the comments and the exhaustive review. After taking a deeper look into the CI workflow, the scripts I had created, and based on all your comments, I decided that, as your mentioned, it makes totally more sense to keep npm-related scripts under package.json for several some important reasons.

  • The error related to npm-s and run-p is not encountered on the scripts because npm resolves them inside node_modules/.bin so at the moment we run the commands on the container, these are not found. Even we can install npm-s and run-p globally, or use npx , keeping npm-related scripts in the package.json would be simpler and cleanner.
  • Most of the scripts I had initially moved into tasks.py were added back into package.json due to other dependent scripts.
  • Almost all the scripts are part of the CI pipeline and referenced to each other, on build and tests stages, also as part of the continous-integration.yml file, so as you highlighted on some scripts I missed to add back after removal, most of them need to be kept in package.json.

Final version and changes

  • As per Jhonatan issue, I made sure playwright and other dependencies are installed in the dev Docker build, and confirmed copy-db.js command is on tasks.py.
  • I removed docker:up and docker:down as duplicated commands.
  • I removed the scripts I initially created on tasks.py, and added back to package.json the npm-related commands.

@mmmavis let me know your comments :)

Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

@dlopezvsr Thank you for walking me through the changes step by step!

@dlopezvsr dlopezvsr merged commit 4339119 into main Oct 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants