Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Fix github url strings (org edx -> openedx) #1358

Merged
merged 2 commits into from
Sep 14, 2022
Merged

Conversation

sarina
Copy link
Contributor

@sarina sarina commented Sep 7, 2022

This PR was autogenerated

This pr replaces the old GitHub organization, github.com/edx, with the new GitHub organization, github.com/openedx.

Tagging @openedx/tcril-engineering for review, but others are welcome to provide review.

Ref: openedx/axim-engineering#42

@sarina sarina force-pushed the tcril/fix-gh-org-url branch 2 times, most recently from 1a63747 to 7f61fca Compare September 12, 2022 02:53
@sarina
Copy link
Contributor Author

sarina commented Sep 12, 2022

@pomegranited - any thoughts on why this failed with ImportError: No module named 'edx_lint' from this set of changes?

pylintrc Outdated
load-plugins=pylint.extensions.redefined_variable_type
ignore =
persistent = yes
load-plugins = edx_lint.pylint,pylint_django,pylint_celery
Copy link
Contributor

Choose a reason for hiding this comment

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

@sarina This is the line that's causing your edx_lint error. Bad news? The pylint==2.4.4 constraint is so old, that we have to go back to edx-lint==1.5.2 to support it, and that version of edx-lint doesn't have a pylint plugin yet.

Worse news? The edx_lint error reported is at the vanguard of a whole army of pylint errors that arise from the other changes in this file.

So you can either remove the alterations to this pylintrc file from this PR, or put this PR on hold until I upgrade pylint and fix all the quality issues that arise. I'll put a task in my next sprint to address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha! I ran edx_lint on every repo because all generated versions of the file have an edx/ url defined. I just checked and the (probably very-old) version of pylintrc on this repo does'n actually have the string, so I can revert this change.

I don't know exactly how valuable it would be to update pylint & use the standard generated pylint file 🤷🏻‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah me neither really.. but eventually the old pylint will bite us. I'll do it as part of the dependabot updates that are pending here, and at least we'll get those in too.

There's likely to be similar issues on the other edx-analytics repos, so feel free to ping me on those if you encounter them?

Thanks @sarina !

@sarina
Copy link
Contributor Author

sarina commented Sep 13, 2022

In fact looking at the build output @pomegranited I think there's a bunch of other packages that would be more worth upgrading such as

  25hnpm WARN deprecated core-js@2.6.12: core-js@<3.4 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Please, upgrade your dependencies to the actual version of core-js.

  25hnpm WARN deprecated bower@1.3.12: This Bower version has SECURITY BUG THAT ALLOWS TO WRITE TO ARBITRARY FILE ON YOUR COMPUTER when you install malicious package. Please upgrade Bower to at least version 1.8.8 if you don't want to get hacked. More info: https://snyk.io/blog/severe-security-vulnerability-in-bowers-zip-archive-extraction/

  25hnpm WARN deprecated natives@1.1.6: This module relies on Node.js's internals and will break at some point. Do not use it, and update to graceful-fs@4.x.

and a ton of other deprecation warnings you can see in the Actions output. I know nothing about this repo tbh so I'm not gonna dictate what to do, I just wonder if it'd be more impactful to address these.

Anyway this is ready for a review!

Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

👍 -- one nit on the CLA link, but this is good to go once that's addressed.

Thanks @sarina :)

  • I tested this by reading through the updated files and checking that all the links work.
  • I read through the code
  • I checked for accessibility issues N/A
  • Updates documentation
  • Commit structure follows OEP-0051

README.md Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants