-
Notifications
You must be signed in to change notification settings - Fork 13
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
🐛 Adding ability for the Maven object to create the settings file #43
Conversation
3044909
to
bedb7f9
Compare
d25e826
to
ebbfccf
Compare
repository/maven.go
Outdated
if err != nil { | ||
err = liberr.Wrap( | ||
err, | ||
"path", | ||
path) | ||
} | ||
_ = f.Close() | ||
addon.Activity("[FILE] Created %s.", path) | ||
addon.Activity("[FILE] Created %s. -- file: %s", path, settings) |
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 cannot include the settings file in the task activity log because it contains credentials.
repository/maven.go
Outdated
return | ||
} | ||
|
||
// | ||
func (r *Maven) injectCacheDir(settings mxj.Map) (mxj.Map, error) { |
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.
mxj.Map isA map which are passed/modified by reference. I don't see the point in returned it.
All of the core projects use named return values by convention. Please use named return values for consistency.
repository/maven.go
Outdated
func (r *Maven) injectProxy(id *api.Identity) (s string, err error) { | ||
s = id.Settings | ||
m, err := mxj.NewMapXml([]byte(id.Settings)) | ||
func (r *Maven) injectProxy(id *api.Identity) (mxj.Map, error) { |
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.
All of the core projects use named return values by convention. Why did you change this?
Also, my previous comment requested the injectProxy() take a mxj.Map to be consistent with injectCacheDir() and because both the m2 dir and proxy settings should be set on either the emptySettings or the setting in the Identity. Right?
Last I checked, ubi9 contains Go 1.18 so we have been development with it to ensure compatibility. I assume the widespread reformatting was because you used gomft 1.19+ which has, imho, become rather draconian. If so, that's fine, all of the files will get reformatted when the project updates Go. If not, please revert the unrelated changes. |
368ab6b
to
29afdb4
Compare
…local repo set Signed-off-by: Shawn Hurley <shawn@hurley.page>
29afdb4
to
3e21fd7
Compare
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.
LGTM
Thanks @shawn-hurley !
We are also adding ability to set the local repository to the M2Dir.