-
Notifications
You must be signed in to change notification settings - Fork 543
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
[report|collector|policies] Add an S3 protocol upload option #3447
[report|collector|policies] Add an S3 protocol upload option #3447
Conversation
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
@TurboTurtle The x86_64 RPM build succeeds, however, I don't seem to be able to access repo metadata for installation and testing:
|
As Fedora 39 is out, let bump its versions in copr builds and GCE testing. Sadly, GCE images offer F39 beta only, so far.. Resolves: sosreport#3448 Relevant: sosreport#3447 Signed-off-by: Pavel Moravec <[email protected]>
The I opened #3448 to fix that. |
NACK to the plain extension of install requirements. We should keep the hard requirements as small as possible, to allow installation of E.g. here, Rather make the requirement and import conditional, like: https://github.com/sosreport/sos/blob/main/sos/policies/distros/__init__.py#L29-L33 |
The deb build is failing, as it does some unit tests as part of the build (That's how deb packaging works). So, we will need to add
For the snap (as that will then fail too), can you add @dnegreira FYI, does that sound reasonable for the
|
e70b5a2
to
e5c2442
Compare
@arif-ali Would adding a Recommends and placing it there be more appropriate in case Universe is not enabled? |
For now, I'll spin up a new F38 VM until the F39 issues are resolved. |
could be an option, but want a +1 from my other fellow colleagues who take on the Ubuntu packaging. But like @pmoravec mentioned, we ought to have a condition of the From the Ubuntu perspective, we expect this to be available for 20.04, 22.04, 23.04, 23.10 and the upcoming 24.04. All of these have But happy on the snap side, as that would be the only way to enable it, and it builds on |
@pmoravec The instructions from packit-as-a-service don't seem to work as described for F38 either:
Since it was identical behavior to F39 I enabled
|
I think we need to change https://github.com/sosreport/sos/blob/main/.packit.yaml#L17-L21 to target I bumped my PR that way and now I see more packit builds \o/ . |
Relevant: sosreport#3447 Relevant: sosreport#3448 Signed-off-by: Pavel Moravec <[email protected]>
Hi @arif-ali and @TrevorBenson,
Unfortunately, both recommendations are not allowed as sosreport is in Ubuntu's main repositories. The condition to load boto3, if it is available, LGTM as a way forward. |
5a49077
to
619b7de
Compare
Relevant: sosreport#3447 Relevant: sosreport#3448 Signed-off-by: Pavel Moravec <[email protected]>
I got a BAD key ID on F38, I originally suspected it was related to this. However I didn't get any key issues on F39 when installing. I didn't check if F39 had an additional change and it now uses something only compatible with >=F39. Just letting you know in case you want to drop F38 since it does not appear to be testable. |
4519d65
to
4c81edd
Compare
I think this may be ready to convert from draft to review. I've tested
For my F39 workstation/laptop I am having some issues with testing I do however welcome anyone with interest in uploads to s3 buckets to give a Packit RPM a try in case I overlooked anything by performing each test manually. |
4c81edd
to
843c65e
Compare
As Fedora 39 is out, let bump its versions in copr builds and GCE testing. Sadly, GCE images offer F39 beta only, so far.. Resolves: sosreport#3448 Relevant: sosreport#3447 Signed-off-by: Pavel Moravec <[email protected]>
Relevant: sosreport#3447 Relevant: sosreport#3448 Signed-off-by: Pavel Moravec <[email protected]>
a77f678
to
437fe69
Compare
Enterprise LinuxRepositories@pmoravec Commit 437fe69 extends the TestingThe 4.6.0-5 current el8 RPM was newer than the 4.6.0-1 from COPR. I suspect this may be an artifact due to Red Hat backporting fixes. I couldn't seem to target it specifically as a downgrade either:
In any case, I was able to avoid removing the dependencies and get it installed via:
However, given the simplicity of the Packit suggestion I'm not sure this is the intended way to install on EL distros w/ COPR, or just the quick workaround I came up with. Did I overlook an option to Packit that would have based the RPM off the current upstream NEVRA when generating the RPM for that COPR repository? I'll clean up this PR by dropping cherry-picks, or commits moving to a new PR, before converting this from Draft to Ready once I know your preference. Thanks |
2c55a4d
to
0a0a158
Compare
@arif-ali Do actions/workflows build a Debian PKG for testing? I've tested Fedora and Enterprise Linux with success. I have not tested on Ubuntu distro's though since there were no suggestions similar to Packit. |
Yes it does, below is the deb artifact from the cirrus builds and was built on Ubuntu 23.10 (mantic), which you could try https://api.cirrus-ci.com/v1/artifact/task/5584901588647936/packages/sos_cirrus.deb Alternatively, a snap is also created, and below is the link to the snap that as created as part of the build https://api.cirrus-ci.com/v1/artifact/task/6710801495490560/packages/sosreport_test_amd64.snap |
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.
Overall it looks good from a code perspective, some minor notes below.
I'm kind of torn on the Recommends
- I fully get the intent (and certainly not having it as a hard Requires
), but afaik most rpm-based distros default to "always install recommends packages" and I wonder if pulling in boto3 as, effectively, a default is the right choice.
I'll make whatever change is suggested. However, isn't the benefit of using Recommends instead of Requires that the end user could bypass this package altogether when using |
Oh I'm almost certainly being too paranoid about this :). My new job has just happened to push package set security (i.e. "as little as possible") to the front of my mind at this point in time, that's all. Taking a bit of a step back from that, you're right. Anyone who actually cares about this will either know about weak deps or have already disabled them. No issue from me on the spec file anymore. |
@arif-ali @TurboTurtle Just as an FYI I'd prefer this hold off from merging until after #3463 is finalized/merged and I can include an additional commit extending this branch to include Thanks! |
Seems I forgot to report back on this the other week. @arif-ali I tested both the deb package and the snap (using |
f5a9a51
to
a5b9539
Compare
@TurboTurtle @arif-ali If not I believe this PR is ready for final reviews. |
I restarted the job - since this particular build is done via Cirrus only Pavel, Arif, Bryn, and myself have permissions to restart it (since the permission handling on non GH-enterprise orgs like us is very coarse - you have to be an "owner" to do it here). The timeout is definitely not related to the changes here, but it does block the other cirrus jobs from running, so I'll watch this and restart it again if needed. |
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.
Code looks good to me, just a stickler point from our Contribution Guidelines. The commit messages should be prefixed with a relevant tag, e.g. [upload]
or somesuch (it makes trawling the git history much easier as time goes on). Also the second commit should be squashed into the first. The S3 obfuscation commit is perfectly fine being a separate commit in the chain.
a5b9539
to
141427d
Compare
Added the context |
I just dont get the
We dont use |
I'll need to check this locally, and see what is going on. Ultimately, the test is being done, once the snap is installed and the snap is based on 22.04's python, and doesn't use the python on 18.04. I will get back on this one, and report back on why this is not working |
On that, below is the error we get on this particular patch, it's not obvious where the issue is, and remember debugging something very similar early last year, I will continue to debug, and get back on the resolution
|
Signed-off-by: Trevor Benson <[email protected]>
Signed-off-by: Trevor Benson <[email protected]>
794619f
to
f5ab86f
Compare
Closes #3112
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines