-
Notifications
You must be signed in to change notification settings - Fork 12
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
misc fixes & linting #31
Conversation
casperdcl
commented
May 13, 2021
•
edited
Loading
edited
- update README
- remove & update outdated info
- misc tidy
- add cross-references
- fix image
- update & simplify workflows
- enable Windows tests
- lint YAML
- rebuild
- fix checks (use local action, restyled config)
- fix Ubuntu 18.04 install
- document deps at https://github.com/iterative/cml#install-cml-as-a-package?
- update & fix version checks (don't require a token when not needed cml#515)
- related: lint & rebuild setup-dvc#18
- depends on windows bugfix Fix pipe reading logic on Windows cml#516 (well, actually found the bug)
- fix checks (use local action, restyled config) - update names - lint YAML - rebuild
- related iterative/setup-dvc#21
oh come on 🌬️ 🚪 🚪 |
Note: iterative/cml#516 was merged and released yesterday |
15ffcb6
to
29df0dc
Compare
apt -y update && apt install -y git | ||
|
||
apt update -y | ||
apt install -y git fontconfig make gcc pkg-config |
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.
should we document these deps?
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.
#23; probably (?)
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.
alternatively we could add this to the action itself?
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.
They're there just because the Ubuntu Docker images are really bare-bones and doesn't include them. We can include them in the action, but that would imply including operating system checks. What if users choose a container with Alpine Linux? 🤔
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 would be safer to either ignore or document them.
- reduce cross-repo duplication and outdated info - add cross-references
steps: | ||
- name: "git" | ||
if: "fromJSON(matrix.runs-on).container == 'ubuntu:18.04'" | ||
- name: install deps |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
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.
Looks great to me! Once you check the camel case style and decide whether to change it or not, I'll approve it.