Skip to content
This repository was archived by the owner on Mar 13, 2024. It is now read-only.

Code Review#79

Merged
coretl merged 1 commit into
mainfrom
review-nov22
Nov 11, 2022
Merged

Code Review#79
coretl merged 1 commit into
mainfrom
review-nov22

Conversation

@gilesknap
Copy link
Copy Markdown
Contributor

@gilesknap gilesknap commented Nov 9, 2022

This is a set of changes as proposed in #75

  • Moved wheel and sdist creation to the 'dist' job
  • Rely on the test matrix to run tests, so remove testing from the container job
  • simplified container build to make runtime only and use the wheel from 'dist'
    • only publish to GHCR for tagged builds
  • review of the requirements-*.txt release assets. Now create separate ones for each of the test matrix
  • fix actions-gh-pages version and don't run it for dependabot
  • other minor improvements and simplifications

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 9, 2022

Codecov Report

Merging #79 (edfc428) into main (dd95a77) will not change coverage.
The diff coverage is n/a.

❗ Current head edfc428 differs from pull request most recent head e17f494. Consider uploading reports for the commit e17f494 to get more accurate results

@@            Coverage Diff            @@
##              main       #79   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           16        16           
=========================================
  Hits            16        16           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment thread .github/workflows/code.yml Outdated
Comment thread .github/workflows/code.yml Outdated
Comment thread .github/workflows/code.yml Outdated
Comment thread .github/workflows/code.yml Outdated
@gilesknap gilesknap requested a review from GDYendell November 9, 2022 15:02
@gilesknap gilesknap force-pushed the review-nov22 branch 5 times, most recently from d5d59c3 to c2db66d Compare November 9, 2022 16:08
Comment thread .github/workflows/code.yml Outdated
Comment thread .github/workflows/code.yml Outdated
@gilesknap
Copy link
Copy Markdown
Contributor Author

I changed back to using head (but kept your modified comment)

python -m $(ls src | head -1) --version

This is because $(ls src) meant that the --version was not getting passed (maybe because it ended up with a trailing line break?)

@gilesknap
Copy link
Copy Markdown
Contributor Author

I think this is ready to merge.
I'm off home.

Comment thread .github/workflows/code.yml
Comment thread .github/workflows/code.yml Outdated
Comment thread .github/workflows/code.yml Outdated
Comment thread .github/workflows/pip_install.sh Outdated
@garryod garryod mentioned this pull request Nov 9, 2022
Comment thread .github/workflows/pip_install.sh Outdated
Comment thread .github/workflows/pip_install.sh Outdated
Comment thread Dockerfile Outdated
Comment thread .github/workflows/code.yml Outdated
Comment thread .github/workflows/docs.yml
Comment thread Dockerfile
@gilesknap gilesknap force-pushed the review-nov22 branch 4 times, most recently from 039c4a0 to 69835dd Compare November 10, 2022 09:14
Comment thread .github/workflows/code.yml Outdated
@gilesknap gilesknap force-pushed the review-nov22 branch 6 times, most recently from c1a0b2d to 2b640e3 Compare November 10, 2022 11:05
Comment thread .github/actions/install_requirements/action.yml Outdated
@gilesknap gilesknap force-pushed the review-nov22 branch 10 times, most recently from 5e9df62 to 541f176 Compare November 10, 2022 11:41
Comment thread .github/workflows/code.yml Outdated
Comment thread .github/actions/install_requirements/action.yml Outdated
Comment thread .github/workflows/pip_install.sh Outdated
@coretl coretl force-pushed the review-nov22 branch 4 times, most recently from b164ee6 to 187a4b2 Compare November 10, 2022 16:11
@coretl coretl marked this pull request as draft November 10, 2022 17:06
Comment thread .github/workflows/code.yml Outdated
@gilesknap gilesknap force-pushed the review-nov22 branch 3 times, most recently from 2a0632f to d0a8b23 Compare November 11, 2022 10:40
@gilesknap
Copy link
Copy Markdown
Contributor Author

@coretl ready to merge (again!) I've verified that release works (you missed 'do' out of your bash for loop)

@coretl coretl force-pushed the review-nov22 branch 2 times, most recently from 52506ee to 09b0ac7 Compare November 11, 2022 13:51
- Moved wheel and sdist creation to the dist job
- Rely on the test matrix to run tests
- Simplified container build to make minimal for build and runtime
  and use wheel from 'dist': only publish to GHCR for tagged builds
- Create separate requirements-*.txt for each of the test matrix
- Fix actions-gh-pages version and don't run it for dependabot
- Move Dockerfile to .devcontainer and use as context to improve
  build times
- Other minor improvements and simplifications
@coretl coretl marked this pull request as ready for review November 11, 2022 14:05
@coretl coretl merged commit 4ccb601 into main Nov 11, 2022
@coretl coretl deleted the review-nov22 branch November 11, 2022 14:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants