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

[Fixes #271] feat: add ci for react #272

Merged
merged 1 commit into from Aug 1, 2021
Merged

Conversation

ADI10HERO
Copy link
Member

Pull Request

What does this PR do?

Fixes #271

What part does this affect?

  • FrontEnd.
  • BackEnd.
  • Documentation.
  • Other. (Please specify below)

CI Workflows.

Before submitting

  • Was this discussed/approved via a GitHub issue or slack?
  • Did you read the contributor guideline?
  • Did you ensure that there aren't any other open Pull Requests for the same update/change?
  • Did you make sure the title is self-explanatory and the description concisely explains the PR?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure the code is clean and docstrings have been added or updated as required?
  • Did you make sure the code is linted/formatted locally prior to submission? (using black and/or prettier)
  • Did you make sure to update the documentation with your changes? (if necessary)

PR review

Anyone in the community is free to review the PR once the tests have passed.

Thank you for contributing to AutoDL. We look forward to your continued support.

Signed-off-by: Aditya Srivastava adityasrivastava301199@gmail.com

@ADI10HERO ADI10HERO requested a review from RusherRG August 1, 2021 10:36
@ADI10HERO ADI10HERO changed the title feat: add ci for react [Fixes #271] feat: add ci for react Aug 1, 2021
Copy link
Member

@RusherRG RusherRG left a comment

Choose a reason for hiding this comment

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

@ADI10HERO looks good to me. The caching part is pretty nice although it seems like recently caching was added to actions/setup-node@v2 itself so should we use that? Reference

@ADI10HERO
Copy link
Member Author

ADI10HERO commented Aug 1, 2021

@ADI10HERO looks good to me. The caching part is pretty nice although it seems like recently caching was added to actions/setup-node@v2 itself so should we use that? Reference

Oh wow, I did not know that...
I will read more on it, maybe compare the time-taken and other features.

@RusherRG
Copy link
Member

RusherRG commented Aug 1, 2021

@ADI10HERO looks good to me. The caching part is pretty nice although it seems like recently caching was added to actions/setup-node@v2 itself so should we use that? Reference

Oh wow, I did not know that. And yeah, it doesn't make sense to cache twice, though does it depend on package-lock or package.json?

It's not about caching twice, it's an optional parameter but since it's already present in actions/setup-node@v2 why not use that it's probably better. Looks like they have just implemented whatever you did using actions/cache in their workflow. actions/setup-node#272

@RusherRG
Copy link
Member

RusherRG commented Aug 1, 2021

Interesting fact: This PR number and the actions/setup-node PR number is the same #272 🤯

@ADI10HERO
Copy link
Member Author

It's not about caching twice, it's an optional parameter but since it's already present in actions/setup-node@v2 why not use that it's probably better. Looks like they have just implemented whatever you did using actions/cache in their workflow. actions/setup-node#272

@RusherRG Haha yeah I got that, so edited the comment but I guess it wasn't reflected on your end.
Anyway, the CI fails when I cache that way because it looks for package-lock.json in root dir, even thought default it changed :(

Signed-off-by: Aditya Srivastava <adityasrivastava301199@gmail.com>
@ADI10HERO ADI10HERO merged commit a927cab into Auto-DL:v1-beta Aug 1, 2021
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.

CI - Github Actions for frontend
2 participants