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.

Sign up for GitHub

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

Jump to bottom

fix(openbsd): services & build tool #4660

Merged
merged 1 commit into from
Dec 7, 2023
Merged

fix(openbsd): services & build tool #4660

merged 1 commit into from
Dec 7, 2023

Conversation

CodeBleu
Copy link
Contributor

@CodeBleu CodeBleu commented Dec 4, 2023 •
edited
Loading

Proposed Commit Message

fix(openbsd): services & build tool

    * Added net/activator for `ifconfig` that is used with OpenBSD
    * Added sysvinit scripts for OpenBSD
    * Updated build-on-openbsd tool with additional dependencies and
      better logic

igalic
igalic suggested changes Dec 4, 2023
View reviewed changes
cloudinit/net/activators.py Outdated Show resolved Hide resolved
cloudinit/net/activators.py Outdated Show resolved Hide resolved
cloudinit/net/ifconfig.py Outdated Show resolved Hide resolved
sysvinit/openbsd/cloudconfig.tmpl Outdated Show resolved Hide resolved
sysvinit/openbsd/cloudinitlocal.tmpl Outdated Show resolved Hide resolved
tools/build-on-openbsd Show resolved Hide resolved
igalic
igalic suggested changes Dec 4, 2023
View reviewed changes
Copy link
Collaborator

@igalic igalic left a comment

Choose a reason for hiding this comment

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

Great progress!
Looking a lot better.
Now it's time to make linters happy, not just reviewers!

cloudinit/net/ifconfig.py Outdated Show resolved Hide resolved
sysvinit/openbsd/cloudfinal.tmpl Outdated Show resolved Hide resolved
sysvinit/openbsd/cloudinit.tmpl Outdated Show resolved Hide resolved
igalic
igalic reviewed Dec 4, 2023
View reviewed changes
Copy link
Collaborator

@igalic igalic left a comment

Choose a reason for hiding this comment

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

reminder, re #4654

tools/build-on-openbsd
echo "Please install python first."
exit 1
fi

# Check dependencies:
depschecked=/tmp/c-i.dependencieschecked
pkgs="
Copy link
Collaborator

Choose a reason for hiding this comment

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

n.b.: following #4654, we can drop the dependency on dmidecode

@CodeBleu CodeBleu force-pushed the main branch 2 times, most recently from 60d3807 to 2067788 Compare December 5, 2023 16:18
@CodeBleu CodeBleu marked this pull request as draft December 5, 2023 17:37
@CodeBleu CodeBleu force-pushed the main branch 2 times, most recently from c1d7099 to 81bc56d Compare December 5, 2023 19:37
@CodeBleu CodeBleu marked this pull request as ready for review December 5, 2023 20:23
@CodeBleu CodeBleu requested a review from igalic December 5, 2023 20:23
igalic
igalic approved these changes Dec 5, 2023
View reviewed changes
Copy link
Collaborator

@igalic igalic left a comment

Choose a reason for hiding this comment

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

👍🏻

@CodeBleu CodeBleu marked this pull request as draft December 5, 2023 21:50
@holmanb holmanb marked this pull request as ready for review December 5, 2023 23:14
@holmanb holmanb self-assigned this Dec 5, 2023
holmanb
holmanb reviewed Dec 6, 2023
View reviewed changes
Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

First pass review, no major objections to this so far @CodeBleu.

I left a few comments and questions inline.

cloudinit/net/ifconfig.py Outdated Show resolved Hide resolved
tools/build-on-openbsd Outdated Show resolved Hide resolved
sysvinit/openbsd/cloudinitlocal.tmpl Show resolved Hide resolved
sysvinit/openbsd/cloudconfig.tmpl Show resolved Hide resolved
igalic
igalic suggested changes Dec 6, 2023
View reviewed changes
Copy link
Collaborator

@igalic igalic left a comment

Choose a reason for hiding this comment

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

one more thing

sysvinit/openbsd/cloudinitlocal.tmpl Show resolved Hide resolved
@CodeBleu
Copy link
Contributor Author

CodeBleu commented Dec 6, 2023

@holmanb @igalic Any objections on just using /etc/rc.local for now? I think it would be good to have something working for the moment, then we can always come back and make it better. I'm going to put this PR back in Draft until I hear back from you all and make changes needed.

@CodeBleu CodeBleu marked this pull request as draft December 6, 2023 01:48
@igalic
Copy link
Collaborator

igalic commented Dec 6, 2023

given all the feedback we've received from the #OpenBSD folks, this seems like the best option

@CodeBleu CodeBleu marked this pull request as ready for review December 6, 2023 11:25
@CodeBleu CodeBleu requested a review from holmanb December 6, 2023 11:25
@CodeBleu CodeBleu force-pushed the main branch 2 times, most recently from 406a19c to d44cb2a Compare December 6, 2023 16:44
igalic
igalic reviewed Dec 6, 2023
View reviewed changes
Copy link
Collaborator

@igalic igalic left a comment

Choose a reason for hiding this comment

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

no need to have the same code in if and else.
Let's just check if rc.local exists in grep for PATH

tools/build-on-openbsd Show resolved Hide resolved
@CodeBleu CodeBleu marked this pull request as draft December 6, 2023 18:52
@CodeBleu CodeBleu marked this pull request as ready for review December 6, 2023 19:33
holmanb
holmanb reviewed Dec 6, 2023
View reviewed changes
Copy link
Member

@holmanb holmanb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great work @CodeBleu. I have another round of questions / requests for you.

My main questions / concerns relate to expectations around how OpenBSD daemons are expected to behave. I'm not an expert in rc, but I just want to make sure that the assumptions held are reasonable, and that I understand them correctly.

cloudinit/net/ifconfig.py Outdated Show resolved Hide resolved
cloudinit/net/activators.py Outdated Show resolved Hide resolved
sysvinit/openbsd/cloudconfig.tmpl Outdated Show resolved Hide resolved
tools/build-on-openbsd Outdated Show resolved Hide resolved
sysvinit/openbsd/cloudconfig.tmpl Show resolved Hide resolved
sysvinit/openbsd/cloudconfig.tmpl Show resolved Hide resolved
tools/build-on-openbsd Show resolved Hide resolved
@CodeBleu CodeBleu force-pushed the main branch 2 times, most recently from a08227f to 99f20a8 Compare December 7, 2023 15:16
@CodeBleu
fix(openbsd): services & build tool
99f20a8 
    * Added net/activator for `ifconfig` that is used with OpenBSD
    * Added sysvinit scripts for OpenBSD
    * Updated build-on-openbsd tool with additional dependencies and
      better logic
@CodeBleu CodeBleu requested a review from holmanb December 7, 2023 16:40
holmanb
holmanb approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@holmanb holmanb 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, thanks for this contribution @CodeBleu!

@holmanb holmanb merged commit 969671b into canonical:main Dec 7, 2023
26 checks passed
@igalic igalic mentioned this pull request Dec 17, 2023
Merged
5 tasks
@hcartiaux hcartiaux mentioned this pull request Oct 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igalic igalic igalic requested changes

@holmanb holmanb holmanb approved these changes

Assignees

@holmanb holmanb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CodeBleu @igalic @holmanb