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(24.04): add wget slices #364

Merged
merged 5 commits into from
Oct 15, 2024

Conversation

nightah
Copy link

@nightah nightah commented Oct 8, 2024

Add slices for gnu wget.

Proposed changes

This change adds the GNU variant of wget.

Forward porting

Assuming this change is approved I'm happy to also PR for 24.10.

Checklist

Additional Context

I have manually tested the installation and usage of the slice.

Copy link

github-actions bot commented Oct 8, 2024

Diff of dependencies:
None found.


@nightah
Copy link
Author

nightah commented Oct 9, 2024

@rebornplusplus I noticed I had to add the email address to my account regarding the CLA, I believe you should be able to re-run the check for a pass now.

Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

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

This looks nice, thank you!

It would be nice to have a spread test for wget actually. See CONTRIBUTING and the existing examples. Please let me know if you need some help with it!

@rebornplusplus
Copy link
Member

@rebornplusplus I noticed I had to add the email address to my account regarding the CLA, I believe you should be able to re-run the check for a pass now.

Thanks! The CLA check is still failing, I suspect due to the database not being updated yet. But that's no problem really.

@nightah
Copy link
Author

nightah commented Oct 9, 2024

It would be nice to have a spread test for wget actually. See CONTRIBUTING and the existing examples. Please let me know if you need some help with it!

I've had a look at the examples, I assume we only really need a singular test to ensure that the binary is working as expected?

@rebornplusplus I noticed I had to add the email address to my account regarding the CLA, I believe you should be able to re-run the check for a pass now.

Thanks! The CLA check is still failing, I suspect due to the database not being updated yet. But that's no problem really.

I've just signed it again this time with my account logged in, not sure if it's going to make a difference.

@rebornplusplus
Copy link
Member

rebornplusplus commented Oct 9, 2024

I've had a look at the examples, I assume we only really need a singular test to ensure that the binary is working as expected?

Yeah, just a smoke test is fine. You could quite simply download any of the chisel.yaml or slice definition files from this repo to check.

@rebornplusplus
Copy link
Member

I've just signed it again this time with my account logged in, not sure if it's going to make a difference.

No worries, our CLA checker hasn't been in the best shape recently. There have been similar problems in the past too.

@nightah
Copy link
Author

nightah commented Oct 9, 2024

@rebornplusplus, this should cover it. I figured the README.md is probably unlikely to change if you think there's a better target file if my assumption is incorrect let me know and I'll adjust it.

@nightah
Copy link
Author

nightah commented Oct 10, 2024

Sorry for all the extra pings, just wanted to clarify that I've been unable to test the integration test via spread and the failure seems to be unrelated to my change with the following error being the end snippet of the failure:

Saving to: ‘chisel.tar.gz’

     0K                                                       100% 9.97M=0s

2024-10-10 07:09:40 (9.97 MB/s) - ‘chisel.tar.gz’ saved [131/131]

FINISHED --2024-10-10 07:09:40--
Total wall clock time: 3.1s
Downloaded: 2 files, 3.5M in 0.03s (101 MB/s)
+ tar -xf chisel.tar.gz -C /usr/local/bin

gzip: stdin: decompression OK, trailing garbage ignored
tar: Child returned status 2
tar: Error is not recoverable: exiting now
-----
.
2024-10-10 18:09:40 Discarding docker:ubuntu-24.04-amd64...
2024-10-10 18:09:41 Successful tasks: 0
2024-10-10 18:09:41 Aborted tasks: 46
2024-10-10 18:09:41 Failed project prepare: 1
    - docker:ubuntu-24.04-amd64:project
error: unsuccessful run

This looks related to the following section of the spread.yaml:

prepare: |
  # Deb arch to GOARCH
  arch="$(dpkg --print-architecture | sed -e 's/armhf/arm/g' -e 's/ppc64el/ppc64le/g')"
  chisel_tar="chisel.tar.gz"

  apt install -y curl wget
  curl -s https://api.github.com/repos/canonical/chisel/releases/latest \
    | awk "/browser_download_url/ && /chisel_v/ && /$arch/" \
    | cut -d : -f 2,3 \
    | tr -d \" \
    | xargs wget -O $chisel_tar

  tar -xf $chisel_tar -C /usr/local/bin

prepare-each: chisel version

suites:
  tests/spread/integration/:
    summary: Tests common scenarios

@linostar
Copy link
Collaborator

@cjdcordeiro can you approve the workflow please?

@linostar
Copy link
Collaborator

Sorry for all the extra pings, just wanted to clarify that I've been unable to test the integration test via spread and the failure seems to be unrelated to my change with the following error being the end snippet of the failure:

Saving to: ‘chisel.tar.gz’

     0K                                                       100% 9.97M=0s

2024-10-10 07:09:40 (9.97 MB/s) - ‘chisel.tar.gz’ saved [131/131]

FINISHED --2024-10-10 07:09:40--
Total wall clock time: 3.1s
Downloaded: 2 files, 3.5M in 0.03s (101 MB/s)
+ tar -xf chisel.tar.gz -C /usr/local/bin

gzip: stdin: decompression OK, trailing garbage ignored
tar: Child returned status 2
tar: Error is not recoverable: exiting now
-----
.
2024-10-10 18:09:40 Discarding docker:ubuntu-24.04-amd64...
2024-10-10 18:09:41 Successful tasks: 0
2024-10-10 18:09:41 Aborted tasks: 46
2024-10-10 18:09:41 Failed project prepare: 1
    - docker:ubuntu-24.04-amd64:project
error: unsuccessful run

This looks related to the following section of the spread.yaml:

prepare: |
  # Deb arch to GOARCH
  arch="$(dpkg --print-architecture | sed -e 's/armhf/arm/g' -e 's/ppc64el/ppc64le/g')"
  chisel_tar="chisel.tar.gz"

  apt install -y curl wget
  curl -s https://api.github.com/repos/canonical/chisel/releases/latest \
    | awk "/browser_download_url/ && /chisel_v/ && /$arch/" \
    | cut -d : -f 2,3 \
    | tr -d \" \
    | xargs wget -O $chisel_tar

  tar -xf $chisel_tar -C /usr/local/bin

prepare-each: chisel version

suites:
  tests/spread/integration/:
    summary: Tests common scenarios

Hey!

It could be that you are running spread on an older system that has a legacy version of tar. Older versions of tar do not support compression method's auto-detection, therefore you will need to extract the archive with tar -xfz, as opposed to tar -xf.

Let me know if that works for you.

@nightah
Copy link
Author

nightah commented Oct 10, 2024

It could be that you are running spread on an older system that has a legacy version of tar. Older versions of tar do not support compression method's auto-detection, therefore you will need to extract the archive with tar -xfz, as opposed to tar -xf.

Let me know if that works for you.

This didn't work, for clarity I'm using the docker backend. So the version of tar should be whatever apt is providing in the container. Also looks like the workflow is failing for the same unrelated reason.

EDIT: I've just seen your updated commit which seems to resolve this, I'll rebase.

@nightah
Copy link
Author

nightah commented Oct 10, 2024

Alright tested this locally successfully now, thanks for fixing the issue.
I believe this is now good to go.

tests/spread/integration/wget/task.yaml Outdated Show resolved Hide resolved
Copy link
Member

@rebornplusplus rebornplusplus 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 to me, thank you!

@rebornplusplus
Copy link
Member

@cjdcordeiro this looks good to be merged.

Copy link
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

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

Nice, thanks! LGTM

@cjdcordeiro cjdcordeiro merged commit fb72190 into canonical:ubuntu-24.04 Oct 15, 2024
14 checks passed
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.

4 participants