Remove github.com/packethost/pkg/env dependency
#324
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jacobweinstock
added
the
do-not-merge
jacobweinstock
changed the title
github.com/packethost/pkg/env dependency
Codecov Report
@@ Coverage Diff @@
## main #324 +/- ##
===================================
- Coverage 19% 18% -2%
===================================
Files 19 17 -2
Lines 1140 1073 -67
===================================
- Hits 222 197 -25
+ Misses 903 862 -41
+ Partials 15 14 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Reorganize files and functionality to live with packages that have the same util. This enables isolating functionality and is a step toward completely removing global variables used in the conf package. Signed-off-by: Jacob Weinstock <[email protected]>
Signed-off-by: Jacob Weinstock <[email protected]>
Reorganize code to be able to remove the packethost/pkg/env dependency. The idea is to follow up with a refactor to clean up the cmd/boot/ package to handle cli flags and env vars in a uniform way. Signed-off-by: Jacob Weinstock <[email protected]>
Signed-off-by: Jacob Weinstock <[email protected]>
Signed-off-by: Jacob Weinstock <[email protected]>
Signed-off-by: Jacob Weinstock <[email protected]>
Some required values were not being passed into the ipxe.Handler struct. Signed-off-by: Jacob Weinstock <[email protected]>
jacobweinstock
force-pushed
the
remove-dep
branch
from
8a111d1
to
e0d3b96
Compare
jacobweinstock
removed
the
do-not-merge
chrisdoherty4
approved these changes
Apr 3, 2023
Comment on lines
+83
to
+84
| err = errors.New("unable to auto-detect public IPv4") | ||
| panic(err) |
There was a problem hiding this comment.
Suggested change
| err = errors.New("unable to auto-detect public IPv4") | |
| panic(err) | |
| panic("unable to auto-detect public IPv4") |
Comment on lines
+92
to
+93
| err := errors.New("PUBLIC_SYSLOG_IP must be an IPv4 address") | ||
| panic(err) |
There was a problem hiding this comment.
Suggested change
| err := errors.New("PUBLIC_SYSLOG_IP must be an IPv4 address") | |
| panic(err) | |
| panic("PUBLIC_SYSLOG_IP must be an IPv4 address") |
| for _, oui := range slice { | ||
| _, err := net.ParseMAC(oui + ":00:00:00") | ||
| if err != nil { | ||
| panic(errors.Errorf("invalid oui in TINK_IGNORED_OUIS oui=%s", oui)) |
Comment on lines
+63
to
+64
| err := errors.New("PUBLIC_IP must be an IPv4 address") | ||
| panic(err) |
There was a problem hiding this comment.
Suggested change
| err := errors.New("PUBLIC_IP must be an IPv4 address") | |
| panic(err) | |
| panic("PUBLIC_IP must be an IPv4 address") |
| i, err := strconv.Atoi(v) | ||
| if err != nil { | ||
| err = errors.Wrap(err, "failed to parse int from env var") | ||
| panic(err) |
| } | ||
| } else { | ||
| // not an IP, panic | ||
| panic("invalid ip cidr in TRUSTED_PROXIES cidr=" + cidr) |
| ignore := map[string]struct{}{} | ||
| for _, ip := range slice { | ||
| if net.ParseIP(ip) == nil { | ||
| panic(errors.Errorf("invalid ip address in TINK_IGNORED_GIS ip=%s", ip)) |
Yeah, exactly. I have another PR thats in the works to clean up all of this. Thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Remove
github.com/packethost/pkg/envdependency and remove unused code. While I tried to stay away from as much refactoring as possible, there is some in here that was needed in order to remove thegithub.com/packethost/pkg/envdependency. Other than this, code was moved around to decrease the cognitive load of understanding this code base and prepare us for a more holistic refactor that will include the deprecation ofgithub.com/packethost/dhcp4-goandgithub.com/packethost/pkgentirely.Why is this needed
Fixes: #
How Has This Been Tested?
Will be doing some manual testing with the Helm chart. Will update here with the results, once complete.
How are existing users impacted? What migration steps/scripts do we need?
Checklist:
I have: