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

Add Ubuntu 22.04 support #268

Merged
merged 1 commit into from May 10, 2023
Merged

Conversation

skylar2-uw
Copy link

Pull Request (PR) description

This PR adds support for Ubuntu 22.04, based on the existing support for Ubuntu 20.04. I believe there are already test cases for Ubuntu 20.04 so I would expect they would continue to pass for 22.04.

This Pull Request (PR) fixes the following issues

n/a

@smortex
Copy link
Member

smortex commented Feb 23, 2023

@skylar2-uw can you please add this OS to metadata.json so that the CI runs against it?

@skylar2-uw
Copy link
Author

@skylar2-uw can you please add this OS to metadata.json so that the CI runs against it?

Oops, sorry, missed that step. Should be good to go now.

@kenyon kenyon changed the title V600 ubuntu 2204 Add Ubuntu 22.04 support Feb 23, 2023
@kenyon kenyon added the enhancement New feature or request label Feb 23, 2023
@smortex
Copy link
Member

smortex commented Feb 23, 2023

You can probably ignore the CentOS 7, but other the CI tests seems relevant: either the code or the tests needs to be adjusted.

@bastelfreak
Copy link
Member

I fixed the CentOS failures in #256. Please rebase against our latest master branch.

@skylar2-uw
Copy link
Author

I fixed the CentOS failures in #256. Please rebase against our latest master branch.

Thanks, @bastelfreak ! I've rebased and extended the existing 20.04 coverage to 22.04. At least in my local testing there's a failing test case with File[snmpd.sysconfig] only containing SNMPDRUN=yes but not SNMPDOPTS='-Lsd -Lf /dev/null -u Debian-snmp -g Debian-snmp -I -smux -p /var/run/snmpd.pid', but in production I do see both set in /etc/default/snmpd so it's possible there's something off with my rake setup (I'm still a beginner). Let me know if anything else needs tweaking.

@smortex
Copy link
Member

smortex commented Feb 25, 2023

Nice! Can you please combine your commits in a single one?

From your working directory:

git rebase -i origin/master # Interactively rewrite history

This will bring your editor listing all commits in your branch. Keep the first line unchanged, and replace pick with squash for all subsequent lines. Save and quit, your editor will pop again and allow you to combine all the commit messages in a single one.

Then push your updated branch:

git push -f            # Send the changes (-f is required because we re-wrote history)

@skylar2-uw
Copy link
Author

Nice! Can you please combine your commits in a single one?

From your working directory:

git rebase -i origin/master # Interactively rewrite history

This will bring your editor listing all commits in your branch. Keep the first line unchanged, and replace pick with squash for all subsequent lines. Save and quit, your editor will pop again and allow you to combine all the commit messages in a single one.

Then push your updated branch:

git push -f            # Send the changes (-f is required because we re-wrote history)

Certainly, thanks a bunch for walking me through this!

@smortex
Copy link
Member

smortex commented Feb 25, 2023

Oops, it looks like your origin/master point to your repo and is not in sync with this one.

No worries, we'll fix that :-)

Assuming git remote -v only list origin that points to your fork, let's add a remote named 'upstream' and update it:

git remote add upstream https://github.com/voxpupuli/puppet-snmp
git fetch upstream

Then we can rebase (same as before, your editor will pop. Don't forget to remove all boilerplate generated by git and just write e.g. "Add Ubuntu 22.04 support" as commit message) and push:

git rebase -i upstream/master
git push -f

The "commits" tab should then only show one commit 👍

Thanks!

@skylar2-uw
Copy link
Author

Oops, it looks like your origin/master point to your repo and is not in sync with this one.

No worries, we'll fix that :-)

Assuming git remote -v only list origin that points to your fork, let's add a remote named 'upstream' and update it:

git remote add upstream https://github.com/voxpupuli/puppet-snmp
git fetch upstream

Then we can rebase (same as before, your editor will pop. Don't forget to remove all boilerplate generated by git and just write e.g. "Add Ubuntu 22.04 support" as commit message) and push:

git rebase -i upstream/master
git push -f

The "commits" tab should then only show one commit +1

Thanks!

OK, rebased and pushed!

Skylar

@skylar2-uw
Copy link
Author

I'm finally getting back to this, and trying to make sense of the couple test failures that are in here. It looks like one problem is here in manifests/init.pp, with this corresponding test case. I think the problem might be that the Ubuntu 22.04 package has TRAPDOPTS already set. Do we want to remove/disable that from the package, or update the test case not to care for Ubuntu 22.04+?

For the second failure, it looks like it's a user mismatch (snmp vs Debian-snmp) but I'm not sure if the intent for the module is to use its own user (snmp) or the OS user (Debian-snmp). Let me know and I think I can figure out how to tweak things accordingly.

Thanks!

@kenyon
Copy link
Member

kenyon commented May 5, 2023

In general we want to keep things as close to the distribution packages as possible.

@skylar2-uw
Copy link
Author

In general we want to keep things as close to the distribution packages as possible.

Thanks for the clarification! I think I've got the Ubuntu 22.04 test cases fixed, though there's now some problems with Debian 11. I don't think I would have done anything that would affect that, but unfortunately don't have a Debian box to test on. Let me know if there's anything I can do to track down the problem, though.

@kenyon
Copy link
Member

kenyon commented May 8, 2023

@skylar2-uw maybe this broke Debian 11: #268 (comment)

@skylar2-uw
Copy link
Author

Thanks for catching that, should be good to go now!

Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Needs a rebase and better commit message, but otherwise seems fine.

@skylar2-uw
Copy link
Author

Needs a rebase and better commit message, but otherwise seems fine.

Thanks, @kenyon! I've rebased and set a more descriptive commit message.

Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Not sure what happened with your rebase, but looks like there are unrelated changes in there now.

@skylar2-uw
Copy link
Author

Not sure what happened with your rebase, but looks like there are unrelated changes in there now.

I'm not sure either... I ran git rebase -i origin/master, set all but the first line to squash, set the commit message, and then git push -f. I think that's the same process as I did before?

@kenyon
Copy link
Member

kenyon commented May 9, 2023

You want to rebase against upstream/master. I just did it and it looks good, but I can't push to your branch. If you change the permissions on this pull request so that maintainers can push to it, I can fix it.

@skylar2-uw
Copy link
Author

You want to rebase against upstream/master. I just did it and it looks good, but I can't push to your branch. If you change the permissions on this pull request so that maintainers can push to it, I can fix it.

Unfortunately I'm not seeing that option, I think because the fork is in our org account rather than a personal account. Is it sufficient just to re-do the rebase against upstream/master?

@kenyon
Copy link
Member

kenyon commented May 9, 2023

Yes, that's all I did, git fetch upstream; git rebase upstream/master.

@skylar2-uw
Copy link
Author

Yes, that's all I did, git fetch upstream; git rebase upstream/master.

Thanks! Think I have all that sorted now. Sorry for the trouble, old CVS/SVN hand trying to get used to the git world...

@kenyon
Copy link
Member

kenyon commented May 9, 2023

There are still unrelated commits. I think it's because your upstream/master is not up to date.

@skylar2-uw
Copy link
Author

Weird... git fetch upstream reports no changes, and git remote -v|grep ^upsteram looks OK

upstream        https://github.com/voxpupuli/puppet-snmp (fetch)
upstream        https://github.com/voxpupuli/puppet-snmp (push)

Is there something obvious I'm missing? Worst case I could re-create the PR against the current master, though if there's a process I can improve for next time, let me know.

@skylar2-uw
Copy link
Author

Aha, think I figured it out, was doing git push rather than git push -f. Think it's working now!

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

LGTM!

@smortex smortex requested a review from kenyon May 9, 2023 23:00
@kenyon kenyon merged commit a2df87f into voxpupuli:master May 10, 2023
20 of 22 checks passed
@skylar2-uw skylar2-uw deleted the v600_ubuntu_2204 branch May 10, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants