Skip to content

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

Merged
ADI10HERO merged 1 commit intoAuto-DL:v1-betafrom
ADI10HERO:ga-react
Aug 1, 2021
Merged

[Fixes #271] feat: add ci for react#272
ADI10HERO merged 1 commit intoAuto-DL:v1-betafrom
ADI10HERO:ga-react

Conversation

@ADI10HERO
Copy link
Copy Markdown
Member

@ADI10HERO ADI10HERO commented Aug 1, 2021

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 [email protected]

@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
RusherRG
RusherRG reviewed Aug 1, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

ADI10HERO commented Aug 1, 2021 •
edited
Loading

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

ADI10HERO commented Aug 1, 2021

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 :(

@ADI10HERO
feat: add ci for react
1c97ae3 
Signed-off-by: Aditya Srivastava <[email protected]>
@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

Reviewers

@RusherRG RusherRG RusherRG approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CI - Github Actions for frontend

2 participants

@ADI10HERO @RusherRG