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

test mdlinter change #286

Merged
merged 4 commits into from
May 28, 2021
Merged

test mdlinter change #286

merged 4 commits into from
May 28, 2021

Conversation

ValarDragon
Copy link
Member

Looking into why mdlinter isn't working, making a PR just to get it to run


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@ValarDragon ValarDragon changed the title test mdlinter chnage test mdlinter change May 28, 2021
@ValarDragon
Copy link
Member Author

hrmm, looks like its working fine on master, maybe it only fails on PR's from outside the repo?

@weikengchen
Copy link
Member

weikengchen commented May 28, 2021

Yes. I previously fixed the linter failure by upgrading the linter docker image version (due to a bug related to GCC, #283). However, it is unable to handle #259 since it is in a different repo, and it seems that currently linter is unable to get that (linter pulls this original repo, instead of the repo where the PR resides, and then tries to switch the branch).

@ValarDragon
Copy link
Member Author

Thanks for fixing that in 283!

I'm a bit confused why the correct branch doesn't get checked out, since the github actions checkout should handle going to the fork

@weikengchen
Copy link
Member

A possibility, which I am totally unsure about, is whether the pull-request parameters are omittable? Since ci.yml does not have them.

https://github.com/arkworks-rs/algebra/blob/master/.github/workflows/mdlinter.yml#L10

(I am totally using heuristics here.)

@ValarDragon
Copy link
Member Author

Lets try removing that, and see if it fixes anything

@ValarDragon ValarDragon marked this pull request as ready for review May 28, 2021 19:25
@weikengchen
Copy link
Member

I have the feeling that we need to merge this in order to test. Merge?

@ValarDragon
Copy link
Member Author

Yeah, lets merge this, and then check it on the next PR from outside the repo

@ValarDragon ValarDragon merged commit bc1e1ce into master May 28, 2021
@ValarDragon ValarDragon deleted the dev/investigate_mdlinter branch May 28, 2021 20:24
@weikengchen
Copy link
Member

Based on my rerunning of https://github.com/arkworks-rs/algebra/runs/2697545270, nope.

I will we will revisit this issue when we have more ideas.

@weikengchen
Copy link
Member

The current idea seems to be this: (1) before docker super-linter, the branch has been switched into an intermediate branch created by GitHub, like pull/279/merge (2) super-linter wants to build a list of files in master, and then sees which files have been changed.

https://github.com/github/super-linter/blob/3dc85fc3bc1f47c6cf14831c666f569b153e9fb9/lib/functions/buildFileList.sh#L67

And the failure seems to be right there.

One possibility: We can fix this by setting VALIDATE_ALL_CODEBASE = true.

@ValarDragon
Copy link
Member Author

Thanks for tracing that, lets set that to true then

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