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

Also test against Tahoe-LAFS master on CI #285

Merged
merged 8 commits into from
Jan 14, 2022

Conversation

exarkun
Copy link
Collaborator

@exarkun exarkun commented Jan 11, 2022

Fixes #281

@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #285 (7aadcfe) into main (14cc987) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
- Coverage   91.76%   91.72%   -0.05%     
==========================================
  Files          51       51              
  Lines        4689     4689              
  Branches      645      645              
==========================================
- Hits         4303     4301       -2     
- Misses        338      339       +1     
- Partials       48       49       +1     
Impacted Files Coverage Δ
src/_zkapauthorizer/model.py 94.21% <0.00%> (-0.65%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14cc987...7aadcfe. Read the comment docs.

@exarkun exarkun changed the title 281.test against tahoe lafs master Also test against Tahoe-LAFS master on CI Jan 12, 2022
@exarkun exarkun marked this pull request as ready for review January 12, 2022 01:47
Copy link
Contributor

@tomprince tomprince left a comment

Choose a reason for hiding this comment

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

I think for this to be useful, we need to either update the pin when running CI, or at least have a periodic job that does that (and possibly pushes the result if CI passes). Otherwise, I'm not sure what having a separate niv entry does, that we couldn't get by updating the current one and pushing to CI.

I think I preference would be the third option (run a job in CI periodically, that update the pin and pushed that change if CI passes).

CONTRIBUTING.rst Outdated Show resolved Hide resolved
@exarkun
Copy link
Collaborator Author

exarkun commented Jan 12, 2022

I think for this to be useful, we need to either update the pin when running CI, or at least have a periodic job that does that (and possibly pushes the result if CI passes). Otherwise, I'm not sure what having a separate niv entry does, that we couldn't get by updating the current one and pushing to CI.

My plan for now is to manually do the update once a week (similar to how we do the nixpkgs update elsewhere). I'm not sure I want to build GitHub-based automation for this update since ideally we'll move the project to GitLab.

@exarkun exarkun closed this Jan 12, 2022
@exarkun exarkun reopened this Jan 12, 2022
@exarkun
Copy link
Collaborator Author

exarkun commented Jan 12, 2022

I pushed some doc additions to record the current and future plans.

CONTRIBUTING.rst Outdated
It is also possible to pass ``pull/<pr-number>/head`` to test against a specific PR.
When feasible we also test against a released version of Tahoe-LAFS.

The Nix package depends on a version of Tahoe-LAFS determined by the "tahoe-lafs" niv entry.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that "nix packaging" == "what is deployed on storage nodes", but it might make sense to be explicit about this.

Maybe something like "The package deployed on our storage node depends on the version of tahoe indicated by the tahoe-lafs niv entry..."?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would also make sense to have this section first, given that it is the one that is consumed outside of CI.

.. note::

Since tahoe-lafs doesn't have correct version information when installed from a github archive,
the packaging in ``default.nix`` includes a fake version number.
This will need to be update manually at least when the minor version of tahoe-lafs changes.

If you want to test multiple versions, you can add an additional source, pointing at other version
If you want to test additional versions, you can add an additional source, pointing at other version.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd be inclined to put the tahoe-lafs-master instructions after this, partly as an example of testing against additional versions.

@exarkun exarkun merged commit 744a063 into main Jan 14, 2022
@exarkun exarkun deleted the 281.test-against-tahoe-lafs-master branch January 14, 2022 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add CI against Tahoe-LAFS master@HEAD
2 participants