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

feat: initial build of github releases support #164

Merged
merged 4 commits into from
Jul 28, 2022

Conversation

rawkode
Copy link
Member

@rawkode rawkode commented Mar 22, 2022

This really needs some integration tests, so I'm going to take a look at something @icepuma shared recently:

https://github.com/aelsabbahy/goss

I'm also not super keen on directory: /usr/local/bin being mandatory and I think we can be smarter about this by peeking into ${PATH} for a XDG /bin directory or /usr/local when missing. Curious what BSD users would expect here too, cc @kraileth and @martintc

Though this PR is available for testing

Closes #24

@rawkode rawkode requested a review from icepuma March 22, 2022 12:42
@rawkode
Copy link
Member Author

rawkode commented Mar 22, 2022

cc @vielmetti @bketelsen

@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2022

Codecov Report

Merging #164 (4dcc241) into main (c4705ca) will decrease coverage by 1.41%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
- Coverage   46.07%   44.66%   -1.42%     
==========================================
  Files          69       70       +1     
  Lines        2179     2248      +69     
==========================================
  Hits         1004     1004              
- Misses       1175     1244      +69     
Impacted Files Coverage Δ
lib/src/actions/binary/github.rs 0.00% <0.00%> (ø)
lib/src/actions/mod.rs 20.28% <0.00%> (-0.61%) ⬇️

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 c4705ca...4dcc241. Read the comment docs.

Step {
atom: Box::new(Chmod {
path: PathBuf::from(format!("{}/{}", &self.directory, &self.name)),
mode: 0o755,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a mode should be chosen which restricts it to the owner, who runs comtrya.

Copy link
Member

@icepuma icepuma left a comment

Choose a reason for hiding this comment

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

One comment and some clippy warnings.

@martintc
Copy link
Contributor

martintc commented Mar 23, 2022

BSD users have a tendency to be a little more strict to sticking to the prescribed hierarchy of files. Best way to get a feel for it is to read the man pages for hier. On NetBSD, I don't think I've ever set up a /usr/local/bin folder. Usually packages added to a NetBSD system using pkgin or pkgsrc, all packages install to /usr/pkg/bin. However this is more specific for how NetBSD feels is best to organize files. I don't run FreeBSD often, @kraileth is more the expert on how things happen on FreeBSD, but if I recall, FreeBSD leans more on using /usr/local. The BSDs also have a different notion as to the purpose of /usr in general. It is seem more as a place where stuff goes isn't part of the base system. Linux has a different model for what /usr is.

@rawkode
Copy link
Member Author

rawkode commented Jul 27, 2022

@icepuma @martintc Ready for review

Copy link
Contributor

@martintc martintc left a comment

Choose a reason for hiding this comment

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

Looks good on my end. I do have one comment regarding checking os_info::Bitness. It is reasonable to assume that any machine that doesn't return Bitness:X32 would be 64-bit. But there are some linux distros that os_info will report back wrongly and we can get a different Bitness or just Bitness::Unknown. I doubt we will be getting a Bitness::Unknown from someone running comtrya on their hacked commodore running linux.

If you think adding a branch for this is unnecessary, your good to merge on my end.

lib/src/actions/binary/github.rs Outdated Show resolved Hide resolved
@icepuma
Copy link
Member

icepuma commented Jul 27, 2022

I'll add my review later

@icepuma
Copy link
Member

icepuma commented Jul 28, 2022

Totally optional for this PR

Should we add a checksum check?

actions:
  - action: binary.github
    name: comtrya
    directory: /tmp
    repository: comtrya/comtrya
    checksum_file: comtrya.sha512

This would be optional. If checksum_file is present, we try to download the checksum_file. If the file is present we have to create the checksum of the binary and check it against the content of the checksum_file.

As we autodetect the architecture, the filename might not work and we should add the expected checksum.

@rawkode rawkode merged commit 9221683 into comtrya:main Jul 28, 2022
@rawkode rawkode deleted the feat/github-releases branch July 28, 2022 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Action: Install GitHub Release Binaries
4 participants