-
Notifications
You must be signed in to change notification settings - Fork 1
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
chore: improve local dev and ci/cd #34
Conversation
python3 -m pip install yq --break-system-packages | ||
python3 -m pip install pycurl --break-system-packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if a virtualenv would be a good idea? @mgyorke has been pursing something like this in snyk/cli#5168
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're only using Python to download the specs for the workspace and orchestration APIs, since their SDKs are not public yet. I was hoping soon-ish we'd have some public SDKs so all this scripting in here is not required anymore. Which is why I don't think a more sophisticated implementation here is worth the effort (although virtualenv sounds really cool)
- run: | ||
name: Check if there are any changes | ||
command: | | ||
git status --porcelain=v1 | tee /dev/stderr | wc -l | grep -qE '^ *0 *$$' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dunno exactly what that command is doing... but maybe git diff-index --quiet HEAD --
would be an option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git diff-index
compares the current tree with main
. The command in CircleCI now checks if there are any changes in the code that haven't been comited. So it doesn't look like we can replace it
- [ ] Linted | ||
- [ ] README.md updated, if user-facing | ||
|
||
🚨After having merged, please update the `snyk-ls` and CLI go.mod to pull in latest client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably have a helper script like the one in the CLI for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: Do we expect contributions from external contributors since this is a public repo? If so, we could iterate in a future PR a simple version of an Open Source type template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only public because the CLI and IDEs are public but we don't really expect external contributors
e5766f8
to
24ea6c6
Compare
24ea6c6
to
0332884
Compare
@@ -43,14 +44,21 @@ In order to update the clients you need access to some private Snyk repositories | |||
export GITHUB_PAT=<GITHUB_PAT> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the instructions for internal contributors, and all the steps are clear and working fine. Thank you!
I wonder if we could change the way we make the request to GitHub and instead of adding the token to the URL we could add it to the Authorization Header? Maybe something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we could change this to call:
curl -H "Authorization: token <GITHUB_PATH>" https://api.github.com/repos/snyk/workspace-service/contents/<path>\?ref\=<commit SHA>
What's the reason for this though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because whenever a token is involved in a request, it's a good practice to use the Authorization
header instead of embedding it directly in the URL. Even if this script is just ran in local environment, I think we can still apply the same standard?
A few improvements. Each commit is its own improvement.