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 Part Of #133: Update Dependencies #269

Merged
merged 21 commits into from
Jun 10, 2021

Conversation

mridul-netizen
Copy link
Contributor

@mridul-netizen mridul-netizen commented May 19, 2021

Following dependencies are left to update
image

1.Got this while updating @zeit/ncc but I updated it to the last latest release
@zeit/ncc is no longer maintained. Please use @vercel/ncc instead.

2.While updating @actions/github

  • coverage reduced
  • some tests were failing(31 exactly)
  • Errors like :
    1.Object.setPrototypeOf called on null or undefined
    2.TypeError: GitHub is not a constructor

3.While updating probot most comon error was
"schedule.repository" is not a known webhook name (https://developer.github.com/v3/activity/events/types/)

Explanation

This PR fixes part of #133

Checklist

  • I have successfully deployed my own instance of Oppiabot.
    • You can find instructions for doing this here.
  • I have manually tested all the changes made in this PR following the manual tests matrix.

@mridul-netizen
Copy link
Contributor Author

@jameesjohn @vojtechjelinek I updated the description with logs related to left dependencies, will prefer to cover them in separate PRs.Also please shed some light on usage on probot-stale

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! Just to check did you verify that there are no breaking changes and can you also do testing with your personal repo of some basic Oppiabot features.

Also, this should probably be "Fix part of #133" since we need to also migrate the three remaining libraries.

@mridul-netizen mridul-netizen changed the title Fix #133: Update Dependencies Fix Part Of #133: Update Dependencies May 20, 2021
@mridul-netizen
Copy link
Contributor Author

Thanks, this looks great! Just to check did you verify that there are no breaking changes and can you also do testing with your personal repo of some basic Oppiabot features.

Also, this should probably be "Fix part of #133" since we need to also migrate the three remaining libraries.

@vojtechjelinek @jameesjohn did some testing here mridul-netizen/oppia-bot-test-repo-#12 any specific job you'd like to mention I should test? Other than that I think it's working fine.

@jameesjohn
Copy link
Contributor

I think if the tests pass, you should be fine.

@jameesjohn
Copy link
Contributor

Got this while updating @zeit/ncc but I updated it to the last latest release
@zeit/ncc is no longer maintained. Please use @vercel/ncc instead.

Since this is a dev dependency, I think it's fine. Although I'm not sure changing it to @vercel/ncc would change the workings of the package since they just changed organizations.

Copy link
Contributor

@jameesjohn jameesjohn left a comment

Choose a reason for hiding this comment

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

Just one comment. Otherwise, LGTM!

"newrelic": "^6.6.0",
"axios": "^0.21.1",
"dotenv": "^9.0.2",
"google-auth-library": "^7.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should manually test the CLA features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oppiabot
Copy link

oppiabot bot commented May 25, 2021

Unassigning @jameesjohn since the review is done.

@oppiabot
Copy link

oppiabot bot commented May 25, 2021

Hi @mridul-netizen, it looks like some changes were requested on this pull request by @jameesjohn. PTAL. Thanks!

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

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

LGTM!

@oppiabot
Copy link

oppiabot bot commented Jun 8, 2021

Unassigning @vojtechjelinek since they have already approved the PR.

@mridul-netizen
Copy link
Contributor Author

@jameesjohn PTAL

@oppiabot oppiabot bot assigned jameesjohn and unassigned mridul-netizen Jun 10, 2021
@oppiabot
Copy link

oppiabot bot commented Jun 10, 2021

Unassigning @mridul-netizen since a re-review was requested. @mridul-netizen, please make sure you have addressed all review comments. Thanks!

Copy link
Contributor

@jameesjohn jameesjohn left a comment

Choose a reason for hiding this comment

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

LGTM

@oppiabot
Copy link

oppiabot bot commented Jun 10, 2021

Unassigning @jameesjohn since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Jun 10, 2021
@oppiabot
Copy link

oppiabot bot commented Jun 10, 2021

Hi @mridul-netizen, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@jameesjohn jameesjohn merged commit aeb0c58 into oppia:master Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants