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

Github Actions: fix warnings and dependencies conflicts #872

Closed
wants to merge 2 commits into from

Conversation

Abhirup-99
Copy link
Contributor

No description provided.

This allows the use of pipenv without any errors.
Comment on lines 27 to 29
# Override language selection by uncommenting this and choosing your languages
# with:
# languages: go, javascript, csharp, python, cpp, java
Copy link
Collaborator

Choose a reason for hiding this comment

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

This belongs with the previous section, so we shouldn't change the indent level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed :).

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@Abhirup-99 Thanks for looking into this :) I was considering merging just the top of your change to the codeql action (hence my comment inline), but noted the other issue you referenced. So other than the warnings, this has potential to also affect how effectively the codeql action works.

As you pointed out we can fix the Pipfile to resolve the QL installation issue. However, we need to do a lot more to properly fix the Pipfile - and we don't use it elsewhere in CI right now (pip is much faster - though admittedly due to repeated locking with pipenv). Given that, I'd prefer that we override what looks like QL autodetecting the Pipfile, and use the standard approach we use in the other CI action, at least for now.

More generally we can consider retaining support for Pipfile and ensuring it still works reliably, though there are more options out there - we can discuss that in the stream.

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback area: infrastructure Project infrastructure labels Jan 11, 2021
This removes the warnings due to the lack of
pipfile.lock and the limit of two commits while
testing.
@Abhirup-99
Copy link
Contributor Author

zulipbot add "PR needs Review".

@neiljp
Copy link
Collaborator

neiljp commented Jan 15, 2021

@Abhirup-99 Your codeql commit still included the pipfile change in .gitignore, so I've removed that and just added the checkout change for now.

CodeQL gives an alternative setting where we can specify the dependency installation commands - could you try that approach in this PR perhaps? We can fix the pipfile in another PR after we've discussed it further as I mentioned above.

@Abhirup-99
Copy link
Contributor Author

Abhirup-99 commented Jan 15, 2021

Closing has been superseded by #875 and #876.

@Abhirup-99 Abhirup-99 closed this Jan 15, 2021
@neiljp neiljp added this to the Next Release milestone Jan 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: infrastructure Project infrastructure PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants