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

feat: add support for yarn #490

Closed
wants to merge 5 commits into from
Closed

Conversation

Ninerian
Copy link
Contributor

@Ninerian Ninerian commented Sep 30, 2021

What have I done

Added support for yarn managed projects

Why was this done

The action itself is capable enough to handle both package managers and creating different one only for yarn would be just a duplication. The existing yarn-audit-fix actions aren't good as this one.

Unsolved problems

The problem rooted in a misconfiguration of the checkout action.
The custom user is currently only used when creating the PR. When the action runs again and updates the existing PR with git push --force the Github Actions user is used again. This leads again to not running the checks.

First push:
first push

Force push:
force push

@Ninerian Ninerian force-pushed the yarn-switch branch 6 times, most recently from 36875c9 to 4db5bc4 Compare October 1, 2021 10:10
@Ninerian Ninerian marked this pull request as draft October 1, 2021 13:24
@ybiquitous
Copy link
Owner

@Ninerian Please provide an appropriate PR title and description you want (e.g., background or motivation etc.)

@Ninerian Ninerian changed the title chore(deps): update dependencies feat: add support for yarn Oct 6, 2021
@Ninerian Ninerian marked this pull request as ready for review October 7, 2021 10:22
@ybiquitous
Copy link
Owner

@Ninerian Thank you. I'll review it later.

@ybiquitous
Copy link
Owner

  • Added support for yarn managed projects
  • Added github_user and github_email inputs

The 2 features look nice.
But, I don't like to include multiple features in one pull request, so can you extract "Added github_user and github_email inputs" into another pull request, please?

If you are busy, I can do it for you.

action.yml Outdated Show resolved Hide resolved
@ybiquitous
Copy link
Owner

@Ninerian Sorry for the late reply.

I'm not familiar with Yarn, so I'm concerned about the yarn-audit-fix logic implemented in this PR. It seems there are some problems about yarn-audit-fix as below:

Therefore, I'm not confident to maintain this feature in the future. What do you think?

@Ninerian
Copy link
Contributor Author

Ninerian commented Nov 2, 2021

@ybiquitous I know about the issue and also the package. I don't understand the reason of the missing fix feature and the explanation by the authors. But as the npm audit fix workaround does the job, I'm fine with that solution.
Before I implemented the change, I also thought about using that package in your workflow. But this would have made the change much larger, as your whole reporting logic depends on the new npm version.

The current change is in my opinion small enough to be manageable. It could be reworked to set the package manager with an variable, but that won't change too much on the maintaining side I guess. Also the documentation needs to be updated to make the flaws with yarn and npm more clear (mono-repo support).

As it is your project I leave it up to you. I can also use the fork to have this feature.

@Ninerian Ninerian requested a review from ybiquitous November 2, 2021 07:38
@ybiquitous
Copy link
Owner

@Ninerian OK, I understand your idea and background. Let's try adding this new feature!

First, can you please rebase the main branch to this feature branch?

And, can you write a description on README.md for this feature?

Copy link
Owner

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

The has-yarn package just checks if yarn.lock exists, so let's replace it with our custom logic, instead of adding a new dependency!

https://github.com/sindresorhus/has-yarn/blob/45d143fe0ce34cb6083c29838bfeb39f4012ae88/index.js#L6

@Ninerian
Copy link
Contributor Author

Ninerian commented Nov 4, 2021

You mean by adding another input variable?

@ybiquitous
Copy link
Owner

No. I think it is possible to replace has-yarn with the following local variable:

async function run() {
  const hasYarn = fs.existsSync("yarn.lock");

  // ...
}

Copy link
Owner

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Please remove dist/.DS_Store which is ignored from Git:

Copy link
Owner

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

[question] Is it possible to write a test for the Yarn feature?

@ybiquitous
Copy link
Owner

@Ninerian

I need a smoke test case for the new Yarn mode. Could you please add it to the CI like this?

smoke:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
with:
fetch-depth: 0
- uses: ./

  smoke-yarn: 
    runs-on: ubuntu-latest 
    steps: 
      - uses: actions/checkout@v3 
        with: 
          fetch-depth: 0 
      - uses: ./
        with:
          package_manager: yarn

It's necessary to add also fixture files (e.g. yarn.lock) for the smoke test case.


I'm concerned about potential problems caused by introducing the new feature. Is it more reliable to use the yarn-audit-fix JS API instead of our custom logic?

@ybiquitous
Copy link
Owner

Closing due to stale.

@ybiquitous ybiquitous closed this Jul 7, 2023
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.

2 participants