-
Notifications
You must be signed in to change notification settings - Fork 33
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
(feat) specify root repo path using DEV_REPO_ROOT for debugging #186
base: master
Are you sure you want to change the base?
Conversation
This allows debugging multiple charts repos directly from chart-build-scripts repo Signed-off-by: Alexandre Lamarre <[email protected]> (fix) duplicate getRepoRoot definitions, centralize to one shared abstraction in main.go Signed-off-by: Alexandre Lamarre <[email protected]>
Signed-off-by: Alexandre Lamarre <[email protected]>
Signed-off-by: Alexandre Lamarre <[email protected]>
Signed-off-by: Alexandre Lamarre <[email protected]>
8287a7e
to
92d139e
Compare
logrus.Fatalf("Unable to get current working directory: %s", err) | ||
} | ||
|
||
func CheckRCCharts(repoRoot string) map[string][]string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the main reasons, I've removed os.Getwd() definitions in package functions is to get rid of scenarios where all functions are not sharing the same abstraction for the repoRoot -- in the case where we make changes like this one.
In this PR I'm preferring using getRepoRoot() function defined in main.go and passing it through to sub functions as that seemed to be mostly the standard when Arvind was still working on the project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also have a shared variable in pkg/charts, along the lines of:
func init() {
repoRoot, err := os.Getwd()
if err != nil {
logrus.Fatal(err)
}
DefaultRepoRoot= repoRoot
}
var (
// default value
DefaultRepoRoot = ""
// flag writes to this one
RepoRoot = ""
)
which could get passed in / written to as a cli.Flag, and consumers would use as charts.RepoRoot
, but I opted for simplicity here
func InitDependencies(repoRoot string, rootFs billy.Filesystem, branchVersion string, currentChart string) (*Dependencies, error) { | ||
var err error | ||
|
||
workDir, err := os.Getwd() | ||
if err != nil { | ||
return nil, err | ||
} | ||
workDir := repoRoot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could alternatively pass in just the repoRoot string and construct the billy.Filesystem inside this function to avoid duplicate parameters in function signatures, although I was unsure if the workDir should ALWAYS be the repoRoot in the autobumps/lifecycle code
Note this also allows for embedding git submodules which would be useful for integration tests in |
Allows specifying relative/absolute root repo paths for debugging by developers
correctly pass in configuration.yaml path config flag : places where
defaultChartsScriptOptionsFile
were used should've been usingChartsScriptOptionsFile
insteadioutil.ReadFile
->os.Readfile
ioutil.WriteFile
->os.WriteFile
ioutil.MkdirTemp
->os.Mkdirtemp