Skip to content
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

Merged
merged 4 commits into from
Aug 14, 2024
Merged

Conversation

f-blass
Copy link
Member

@f-blass f-blass commented Aug 14, 2024

better traceability which versions are used.

Copy link
Contributor

@d047491 d047491 left a 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:

  1. Let the uploader read the version from the manifest directly
  2. Or better: parameterize the User-Agent Header value for the uploader (for example add it to the Uploader struct type)

Copy link
Contributor

@d047491 d047491 left a 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

@f-blass
Copy link
Member Author

f-blass commented Aug 14, 2024

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.

Copy link
Contributor

@d047491 d047491 left a 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.

@@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
UserAgent: fmt.Sprintf("cloud-authorization-buildpack/%s", buildpackVersion),
UserAgent: fmt.Sprintf("cloud-authorization-buildpack/%s", s.BuildpackVersion),

Copy link
Contributor

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.

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (s *Supplier) upload(creds *services.IASCredentials, tlsCfg tlsConfig, rootDir, buildpackVersion string) error {
func (s *Supplier) upload(creds *services.IASCredentials, tlsCfg tlsConfig, rootDir) error {

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if err := s.upload(identityCreds, tlsCfg, cfg.Root, s.BuildpackVersion); err != nil {
if err := s.upload(identityCreds, tlsCfg, cfg.Root); err != nil {

Copy link
Contributor

@d047491 d047491 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good now :-)

@f-blass f-blass merged commit 94b9c67 into main Aug 14, 2024
8 checks passed
@f-blass f-blass deleted the version-header branch August 14, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants