-
Notifications
You must be signed in to change notification settings - Fork 6
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
Set buildpack version as header on dcl upload #93
Conversation
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.
I don't fully understand the approach here. There is a setter, that is always called und gets the value from the manifest. It does not look clean to me. I see better ways to achieve that. I did not think it trough yet but. I have two ideas:
- Let the uploader read the version from the manifest directly
- Or better: parameterize the User-Agent Header value for the uploader (for example add it to the Uploader struct type)
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.
reason: see last comment
Originally I wanted to use build flags, which however is not possible with the buildpack packager. I thought about adding it to the Uploader struct aswell. I didn't like it either, because you would need to pass it also to all other structs if required somewhere else. Adding the whole manifest to the uploader seems even worse, because the uploader should not even need to know that it is running in the context of a buildpack. Since the project is rather small and we do not have HTTP calls anywhere else outside of the uploader, I'll add it to the Uploader struct for now. |
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.
just a little beautification.
pkg/supply/supply.go
Outdated
@@ -284,6 +285,7 @@ func (s *Supplier) upload(creds *services.IASCredentials, tlsCfg tlsConfig, root | |||
Root: path.Join(s.Stager.BuildDir(), rootDir), | |||
Client: client, | |||
AMSInstanceID: creds.AmsInstanceID, | |||
UserAgent: fmt.Sprintf("cloud-authorization-buildpack/%s", buildpackVersion), |
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.
UserAgent: fmt.Sprintf("cloud-authorization-buildpack/%s", buildpackVersion), | |
UserAgent: fmt.Sprintf("cloud-authorization-buildpack/%s", s.BuildpackVersion), |
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.
no need to make to provide an attribute as a parameter to a private funtion.
pkg/supply/supply.go
Outdated
@@ -274,7 +275,7 @@ func (s *Supplier) supplyCertCopier() error { | |||
return os.Chmod(destFile, 0755) | |||
} | |||
|
|||
func (s *Supplier) upload(creds *services.IASCredentials, tlsCfg tlsConfig, rootDir string) error { | |||
func (s *Supplier) upload(creds *services.IASCredentials, tlsCfg tlsConfig, rootDir, buildpackVersion string) 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.
func (s *Supplier) upload(creds *services.IASCredentials, tlsCfg tlsConfig, rootDir, buildpackVersion string) error { | |
func (s *Supplier) upload(creds *services.IASCredentials, tlsCfg tlsConfig, rootDir) error { |
pkg/supply/supply.go
Outdated
@@ -101,7 +102,7 @@ func (s *Supplier) Run() error { | |||
return fmt.Errorf("could not write profileD file: %w", err) | |||
} | |||
if cfg.ShouldUpload { | |||
if err := s.upload(identityCreds, tlsCfg, cfg.Root); err != nil { | |||
if err := s.upload(identityCreds, tlsCfg, cfg.Root, s.BuildpackVersion); err != nil { |
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.
if err := s.upload(identityCreds, tlsCfg, cfg.Root, s.BuildpackVersion); err != nil { | |
if err := s.upload(identityCreds, tlsCfg, cfg.Root); err != nil { |
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.
looks good now :-)
better traceability which versions are used.