-
Notifications
You must be signed in to change notification settings - Fork 3
Update ci.yaml #163
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
Update ci.yaml #163
Conversation
Allowing fork-pull workflow
Codecov Report
@@ Coverage Diff @@
## main #163 +/- ##
===========================================
- Coverage 93.73% 36.51% -57.23%
===========================================
Files 16 16
Lines 1835 1835
===========================================
- Hits 1720 670 -1050
- Misses 115 1165 +1050 |
steps: | ||
- name: Checkout branch | ||
uses: actions/checkout@v3 | ||
with: |
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.
Would we also need to remove this for all of the respective integration tests?
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'd say so
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.
Good catch!
More removals
.github/workflows/ci.yaml
Outdated
uses: actions/checkout@v3 | ||
with: | ||
ref: ${{ github.head_ref }} | ||
fetch-depth: 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.
nit: This fetches the entire history. Is this to run some backwards compatibility test?
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.
You mean it should be fetch-depth: 1
? Which most likely is the default I guess.
I don't think there is any good reason why we do 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.
Yeah might be better to just remove the entire with block for simplicity.
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 think fetch-depth
is used in our repositories because setuptools-scm
uses Git tags to determine the version when building and most our projects use it for packaging.
Default depth
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.
Thanks a lot for looking into this!
Allowing fork-pull workflow