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

sdns: Add new package to repo #1457

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

justledbetter
Copy link

I've added the sdns package (https://github.com/semihalev/sdns) to the repo, and confirmed it builds and appears to startup/shutdown correctly. This is my first attempt at doing any of this, so please forgive me if anything is amiss! :)

# source. A copy of the CDDL is also available via the Internet at
# http://www.illumos.org/license/CDDL.

# Copyright 2024 Guo-Rong Koh
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't remember writing this 😂

@justledbetter
Copy link
Author

justledbetter commented May 15, 2024 via email

@gkoh
Copy link
Contributor

gkoh commented May 15, 2024

Im sorry, I scavenged through gh and mattermost and nsd to piece together all the moving parts for this! This is what I get for keeping changes to a minimal.. 😅

No problems!
Happy that my contribution was helpful to you 🙂

Copy link
Member

@hadfl hadfl left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution. There are a few things I'd like you to change. Please get back to me if something does not make sense.

# http://www.illumos.org/license/CDDL.
# }}}

# Copyright 2022 OmniOS Community Edition (OmniOSce) Association.
Copy link
Member

Choose a reason for hiding this comment

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

can you please update the year to 2024

init
clone_go_source $PROG semihalev v$VER
patch_source
print_config
Copy link
Member

Choose a reason for hiding this comment

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

this looks like a leftover from another build.sh

Comment on lines 19 to 20
group groupname=$(PROG) gid=5353
user ftpuser=false username=$(PROG) uid=5353 group=$(PROG) \
Copy link
Member

Choose a reason for hiding this comment

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

system UIDs should be below 100 to avoid clashes with user IDs. have a look at doc/idlist.md and look for a free uid/gid pair (e.g. 58). and document the ID you are using in idlist.md.

This file was based on other projects throughout the omnios-extra, tree - the authors of which I am very thankful for!  😁
Tentatively taking UID 35 (opposite of 53), unless there are objections.
@justledbetter justledbetter requested a review from hadfl May 22, 2024 19:36
@oetiker
Copy link
Member

oetiker commented Jun 5, 2024

ping @hadfl

Copy link
Member

@citrus-it citrus-it left a comment

Choose a reason for hiding this comment

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

Hello, thank you for the pull request and apologies for the delay in looking at this. hadfl won't be able to get back to it for a while so I've taken a look through and left a couple of comments.

Please could you also update the doc/packages.md and doc/baseline files for this new package?

Thanks again.

doc/idlist.md Outdated Show resolved Hide resolved
build/sdns/build.sh Outdated Show resolved Hide resolved
build/sdns/local.mog Outdated Show resolved Hide resolved
build/sdns/files/sdns-template.xml Outdated Show resolved Hide resolved
@oetiker
Copy link
Member

oetiker commented Jul 1, 2024

@gkoh ping

@gkoh
Copy link
Contributor

gkoh commented Jul 1, 2024

@gkoh ping

Sorry, this is not my PR. I only commented because my name was 'used in vain'.

@oetiker
Copy link
Member

oetiker commented Jul 1, 2024

@justledbetter ping!

@justledbetter
Copy link
Author

Apologies, have been out of town! Will get back on this shortly!

@justledbetter
Copy link
Author

Thanks again to everyone for your feedback, and most importantly for your patience! Hopefully things look better after the latest commits :)

@@ -85,6 +85,7 @@
| illumos | 25 | smmsp
| extra | 27 | postfix
| extra | 28 | postdrop
| extra | 35 | sdns
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you misinterpreted the input from @citrus-it. Yes we need this in groups since you allocate the sdns gid. But you'll also need to leave your initial entry in users, as you allocate a uid as well

@hadfl
Copy link
Member

hadfl commented Jul 6, 2024

I think you have overlooked the comments regarding removing print_config which seems to be a leftover from another build where you copied from and also adding this package to doc/packages.md and doc/baseline.

Sorry again, that getting back to this review took so long.

@oetiker
Copy link
Member

oetiker commented Oct 7, 2024

@justledbetter it seems there is still a bit of stuff left seee the last comment form @hadfl !

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.

5 participants