Leon/issue/31/rpm packaging #33
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce RPM packaging support for the project. This includes enhancements to the main Makefile for building source RPMs, new systemd service and timer unit files for certificate renewal, a new RPM spec file, and a dedicated Makefile for COPR builds. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant rpmbuild
participant Systemd
User->>Makefile: make srpm
Makefile->>Makefile: Create tarball from Git HEAD
Makefile->>Makefile: Verify version in spec file
Makefile->>rpmbuild: Build source RPM using spec file
rpmbuild-->>Makefile: Return built SRPM
User->>Systemd: Enable tapir-renew.timer
Systemd->>tapir-renew.timer: Trigger weekly
tapir-renew.timer->>tapir-renew.service: Start service if certs exist
tapir-renew.service->>/usr/bin/tapir-cli: Run certificate renewal
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
cmd/root.go (1)
45-46: Hardcoded path should be configurableWhile adding a default path is good, hardcoding
/etc/dnstapir/tapir-cli.yamlmay cause issues in different environments or platforms.Consider making this configurable through an environment variable:
-const default_TAPIR_CLI_CFG_FILE = "/etc/dnstapir/tapir-cli.yaml" +const default_TAPIR_CLI_CFG_FILE_PATH = "/etc/dnstapir" +const default_TAPIR_CLI_CFG_FILE_NAME = "tapir-cli.yaml" + +func getDefaultConfigFile() string { + if path := os.Getenv("TAPIR_CLI_CONFIG_DIR"); path != "" { + return filepath.Join(path, default_TAPIR_CLI_CFG_FILE_NAME) + } + return filepath.Join(default_TAPIR_CLI_CFG_FILE_PATH, default_TAPIR_CLI_CFG_FILE_NAME) +}Then update the config flag initialization to use this function.
.github/workflows/release.yaml (1)
31-58: Duplicate upload step nameThere are two steps named "Upload Tarball" which can cause confusion.
The first "Upload Tarball" step (which is empty) should be removed, and the remaining upload steps should have clear, unique names.
Makefile (1)
45-52: New binary distribution targetThe
bindisttarget that depends onsrcdistbuilds a binary from the source tarball.Consider adding error checking to handle potential build failures more gracefully:
bindist: srcdist -mkdir -p dist/bin/build cp dist/src/$(PROG)-$(VERSION).tar.gz dist/bin/build/ tar xvf dist/bin/build/$(PROG)-$(VERSION).tar.gz -C dist/bin/build rm -f dist/bin/build/*.tar.gz - cd dist/bin/build/$(PROG) && make build + cd dist/bin/build/$(PROG) && make build || { echo "Build failed"; exit 1; } mv dist/bin/build/$(PROG)/$(PROG) dist/bin/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
dist/.gitignoreis excluded by!**/dist/**dist/rpm/SOURCES/tapir-cli.sysusersis excluded by!**/dist/**dist/rpm/SOURCES/tapir-cli.yamlis excluded by!**/dist/**dist/rpm/SOURCES/tapir-renew.serviceis excluded by!**/dist/**dist/rpm/SOURCES/tapir-renew.timeris excluded by!**/dist/**dist/rpm/SPECS/tapir-cli.spec.inis excluded by!**/dist/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
.github/workflows/release.yaml(1 hunks)Makefile(2 hunks)cmd/root.go(4 hunks)go.mod(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
11-11: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
18-18: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
18-18: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
18-18: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
18-18: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
18-18: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
18-18: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
26-26: string should not be empty
(syntax-check)
28-28: string should not be empty
(syntax-check)
30-30: string should not be empty
(syntax-check)
🔇 Additional comments (13)
go.mod (2)
8-8: Dependency version update looks goodThe updated version of the
github.com/dnstapir/tapirmodule includes newer changes that are likely required for the RPM packaging functionality added in this PR.
1-66:Details
❓ Verification inconclusive
Verify the removed dependency is no longer needed
According to the summary, the dependency on
github.com/google/uuidhas been removed. Please ensure that no code in the project still relies on this library.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining imports of the uuid package in the codebase rg -l "github.com/google/uuid" --type goLength of output: 40
Confirm Removal of github.com/google/uuid
The initial search using
"rg -l 'github.com/google/uuid' --type go"returned no results, which suggests that there are no remaining import statements for this dependency in the Go source files. However, since the search output was completely empty and might not cover all possible code paths (such as non-Go files or indirect references), please perform a manual verification to be certain that no part of the project still relies ongithub.com/google/uuid.
- Double-check all source and test files: Ensure that the removal of this dependency doesn’t affect any indirect usage or build scripts.
- Confirm removal across the entire repository: Consider reviewing any non-Go files or configuration scripts that might reference this dependency inadvertently.
cmd/root.go (6)
20-20: Code simplification looks goodRemoving unnecessary variables (
imr,servername,certname, andProg) and keeping only the essentialcfgFilemakes the code cleaner.
51-53: Changed default behavior to standalone modeThe default for
--standaloneis nowtrue, which changes the application's behavior to not connect to TAPIR-POP by default.Is this change intentional and communicated to users? This could potentially break existing workflows for users who upgrade.
91-98: Simplified configuration loading logicThe configuration file handling has been streamlined, which is good. However, the error handling is now more strict, as it will terminate on any config read error.
This change will make the application terminate if the config file cannot be found. Please confirm this behavior is intentional and expected by users.
105-114: Updated configuration key referencesThe code now directly references
cli.tapir-popin error messages rather than using variables.
119-119: Updated API key configurationThe API key is now retrieved from the updated configuration path.
63-64:Details
✅ Verification successful
New commands added
Two new commands (
EnrollCmdandRenewCmd) have been added, expanding the CLI functionality.
🏁 Script executed:
#!/bin/bash # Check if these commands are properly implemented in the codebase rg -A 5 "var EnrollCmd" --type go rg -A 5 "var RenewCmd" --type goLength of output: 70
🏁 Script executed:
#!/bin/bash echo "Searching the repository for any occurrence of 'EnrollCmd' in Go files..." rg -n "EnrollCmd" --type go echo "Searching the repository for any occurrence of 'RenewCmd' in Go files..." rg -n "RenewCmd" --type goLength of output: 477
CLI Command Registration Verified
In
cmd/root.goat lines 63-64, the new commands (EnrollCmdandRenewCmd) are correctly registered withrootCmd.AddCommand(...). A repository-wide search confirmed their presence, ensuring the CLI expansion is properly implemented..github/workflows/release.yaml (1)
1-5: New release workflow is a good additionAdding an automated workflow for releasing artifacts when a release is published improves the project's CI/CD capabilities.
Makefile (4)
34-36: Enhanced clean targetThe clean target has been improved to remove all distribution-related directories, which is important for the new packaging targets.
41-44: New source distribution targetThe
srcdisttarget creates a source tarball using git archive, which is a clean way to package the source code.
53-58:Details
✅ Verification successful
New RPM packaging target
The
rpmtarget depends onbindistand creates an RPM package, which is good for systems that use RPM package management.
🏁 Script executed:
#!/bin/bash # Check if the RPM spec template file exists find . -name "*.spec.in"Length of output: 61
RPM Packaging Target Verified
The RPM packaging target now appears solid. Verification confirms that the RPM spec template (found as
./dist/rpm/SPECS/tapir-cli.spec.in) exists. The target correctly depends onbindist, creates the necessary directories, copies the binary, processes the spec template withsedto inject the version, and finally callsrpmbuildas intended.No further changes are required.
6-6:Details
✅ Verification successful
Updated build flags
Added
-B gobuildidto the Go build flags.
🌐 Web query:
What does -B gobuildid flag do in Go build flags?💡 Result:
The
-Bflag in Go's build system is used to set the build ID of the generated binary. This flag is passed to the linker (cmd/link) and influences the unique identifier embedded in the executable, which is critical for Go's build caching and reproducibility mechanisms.Key Details:
Purpose:
The build ID is a hash-based identifier computed from the binary's content. It helps the Go toolchain determine if a rebuild is necessary by checking if inputs (or their dependencies) have changed[10].Usage:
When building the Go toolchain itself,-Bcan cause issues because the build ID becomes part of the binary's content. This creates a circular dependency: modifying the build ID alters the binary, which in turn changes the build ID in subsequent compilations[10]. For example:# Hypothetical example (not standard practice) go build -ldflags="-B custom_build_id" .Impact on Reproducibility:
Overriding the build ID with-Bcan disrupt deterministic builds, as the ID is typically derived from the binary's content. Forcing a specific ID may require careful handling in scenarios like toolchain compilation[10].Related Flags:
-ldflags="-buildid=..."is another way to set the build ID during linking[7].- Stripping the build ID (e.g.,
-ldflags="-buildid= -s") removes it entirely, reducing binary size[7].In summary,
-Bdirectly modifies the build ID, which is integral to Go's build caching and dependency tracking. Its use is rare outside specialized contexts like debugging build-system behavior or ensuring specific build fingerprints.Citations:
- 1: https://blog.cyphur.com/what-the-bin-nim-golang-cpp/
- 2: https://pkg.go.dev/cmd/go
- 3: https://www.hitzhangjie.pro/debugger101.io/6-develop-inst-debugger/3-process_start_attach.html
- 4: https://programmingpercy.tech/blog/modify-variables-during-build/
- 5: https://www.hitzhangjie.pro/debugger101.io/5-debugger-skeleton/2-debugger_demo.html
- 6: https://opensource.com/article/22/4/go-build-options
- 7: https://pkg.go.dev/github.com/mjl-/gobuild
- 8: https://blog.howardjohn.info/posts/go-build-times/
- 9: https://groups.google.com/g/golang-nuts/c/6wFhfrlbqYA
- 10: golang/go#63934
Build Flag Update Verified
The addition of the
-B gobuildidflag in the Makefile is correct. As confirmed by our query, this flag explicitly sets the build ID in the binary via the linker, thereby influencing Go's build caching and reproducibility mechanisms. This change is intentional and aligns with our build strategy.File: Makefile
Line: 6Final Snippet:
GOFLAGS:=-v -ldflags "-X app.version=$(VERSION)-$(COMMIT) -B gobuildid"
johanix
left a comment
johanix
left a comment
There was a problem hiding this comment.
I have no opinion on all the RPM stuff, I only understand the go code (and Makefile, etc).
zluudg
force-pushed
the
leon/issue/31/rpm-packaging
branch
2 times, most recently
from
1505472 to
0a623f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Makefile (1)
55-55: Update PHONY targets
The.PHONYdeclaration needs to include the newtarballandsrpmtargets to prevent collisions with files of the same name.
🧹 Nitpick comments (10)
.copr/Makefile (2)
3-5: Markprereqas phony
Add a.PHONY: prereqdirective to avoid collisions with any file namedprereq.
6-7: Marksrpmas phony and reduce verbosity
Add.PHONY: srpmand consider prefixing the$(MAKE)invocation with@to suppress redundant output during automated runs.Makefile (2)
34-39: Consolidate cleanup commands
You can merge the multiplerminvocations into one pattern for clarity, e.g.:clean: @rm -rf $(PROG) *~ cmd/*~ *.tar.gz rpm/{SOURCES/*.tar.gz,BUILD,BUILDROOT,SRPMS,RPMS}
44-46: Quote tarball filename
Wrap$(PROG)-$(VERSION).tar.gzin quotes to guard against unexpected characters, e.g.:- git archive --format=tar.gz --prefix=$(PROG)/ -o $(PROG)-$(VERSION).tar.gz HEAD + git archive --format=tar.gz --prefix=$(PROG)/ -o "$(PROG)-$(VERSION).tar.gz" HEADrpm/SPECS/tapir-cli.spec (6)
15-16: Fix typo in description
ChangeDNSTAPIR EDGE ClI TooltoDNSTAPIR EDGE CLI Toolfor consistent casing.
24-26: Consider using%make_buildmacro
For consistency with other RPM workflows, you may swapmakefor%make_build, though the explicit call is acceptable.
27-36: Explicitly name installed config file
Whileinstall -m 0640 %{SOURCE3} …/dnstapir/drops the YAML into the directory, specifying the full path (e.g.,…/dnstapir/tapir-cli.yaml) improves clarity.
45-48: Combine user/group creation
To reduce boilerplate, merge the twogetentchecks into a single shell block, e.g.:%pre getent group dnstapir || groupadd -r dnstapir getent passwd tapir-renew || useradd -r -d /etc/dnstapir -G dnstapir -s /sbin/nologin tapir-renew
49-56: Remove empty scriptlets until needed
Omit the unused%post,%preun,%postun, and%checksections to keep the spec concise; add them when scripts are required.
57-58: Add initial changelog entry
An empty%changelogmisses history; include at least one entry for version 0.3 to track the initial release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.copr/Makefile(1 hunks).gitignore(1 hunks)Makefile(2 hunks)rpm/SOURCES/tapir-renew.service(1 hunks)rpm/SOURCES/tapir-renew.timer(1 hunks)rpm/SPECS/tapir-cli.spec(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- rpm/SOURCES/tapir-renew.service
- rpm/SOURCES/tapir-renew.timer
- .gitignore
🧰 Additional context used
🪛 RuboCop (1.75.5)
rpm/SPECS/tapir-cli.spec
[fatal] 1-1: unexpected token tCOLON
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
🔇 Additional comments (5)
.copr/Makefile (1)
1-1: Correct project root resolution
Thetopvariable accurately resolves the parent directory of this Makefile, ensuring COPR builds invoke the top-levelMakefile.Makefile (1)
13-13: Validate SPECFILE path
Ensure the spec file atrpm/SPECS/tapir-cli.specexists relative to the repo root and consider quoting the variable (e.g.$(SPECFILE)) if paths may contain spaces.rpm/SPECS/tapir-cli.spec (3)
18-20: Macro fallbacks look good
The%{!?_unitdir}and%{!?_sysusersdir}definitions ensure compatibility across distros.
21-23: %prep section is correct
Using%setup -n %{name}matches the tarball’s root directory.
37-44: Verify file ownership and permissions
You’ve set the binary’s permissions to0770. Confirm this restrictive mode is intended—if wider system access is required, consider0755.
zluudg
force-pushed
the
leon/issue/31/rpm-packaging
branch
from
0a623f8 to
467c366
Compare
zluudg
force-pushed
the
leon/issue/31/rpm-packaging
branch
from
467c366 to
ff1ce0c
Compare
Summary by CodeRabbit