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

[tmsUpload, tmsExport] Provide additional log message on successful upload and export to node #4624

Conversation

artembannikov
Copy link
Contributor

@artembannikov artembannikov commented Oct 10, 2023

Changes

  • Tests
  • Documentation

@artembannikov artembannikov requested a review from a team as a code owner October 10, 2023 14:03
Copy link
Member

@o-liver o-liver left a comment

Choose a reason for hiding this comment

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

The ITs show that the fileInfo can be parsed as expected, right?

cmd/tmsExport.go Show resolved Hide resolved
}
communicationInstance.logger.Info("Node upload executed successfully")

// Important: there are Customers, who might rely on format of this log message to parse transport request id
Copy link
Member

Choose a reason for hiding this comment

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

And we cannot offer them something else, such as variables in the common pipeline environment (CPE)? Maybe additionally to the log message we could also provide the information in CPE variables. Then they can use that in the pipeline, without having to parse the log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And we cannot offer them something else, such as variables in the common pipeline environment (CPE)?

Yes, that will be an ideal case. For now, we have agreed with the Customer, who complained about missing log message after we switched to the new Golang implementation of the step, that we provide a log message with transport request id in the Golang implementation as well.

Usage of variables in CPE or other options, if they are, have to be and will be discussed separately.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's do so. @marcusholl What do you think? Would it help Piper users, if they could retrieve this information from the CPE, rather than parsing the log? Is there something else we could offer, apart from the CPE?

@artembannikov
Copy link
Contributor Author

The ITs show that the fileInfo can be parsed as expected, right?

@o-liver I didn't get you here.

@o-liver
Copy link
Member

o-liver commented Oct 13, 2023

The ITs show that the fileInfo can be parsed as expected, right?

@o-liver I didn't get you here.

Only that this fileInfo object is new to me. Although it has already been in the code from the get go. Just wanted to confirm that this is tested with the ITs. So there is a test that deserializes the response json into this object.

@artembannikov
Copy link
Contributor Author

artembannikov commented Oct 13, 2023

Only that this fileInfo object is new to me. Although it has already been in the code from the get go. Just wanted to confirm that this is tested with the ITs. So there is a test that deserializes the response json into this object.

Obtaining fileInfo with a client method has not been changed: https://github.com/SAP/jenkins-library/pull/4624/files#diff-6cdbba2d0a3877496974969447a6b2050384521e75038b2677bf014e9d47f647R351

Unit tests for the client method are already in place: https://github.com/SAP/jenkins-library/blob/master/pkg/tms/tmsClient_test.go#L379

Output of a wrapping util method has been changed: https://github.com/SAP/jenkins-library/pull/4624/files#diff-6cdbba2d0a3877496974969447a6b2050384521e75038b2677bf014e9d47f647R341

But its functionality is tested with upper layer unit tests of the entire execution logic, e.g.:
https://github.com/SAP/jenkins-library/blob/master/cmd/tmsExport_test.go#L67
https://github.com/SAP/jenkins-library/blob/master/cmd/tmsUpload_test.go#L182

Moreover, we should also have integration tests for the entire flow: https://github.com/SAP/jenkins-library/blob/master/integration/github_actions_integration_test_list.yml#L23

@artembannikov
Copy link
Contributor Author

@o-liver Could you please trigger all the relevant pipelines, so they could check, if I haven't forgotten to change anything.

@o-liver
Copy link
Member

o-liver commented Oct 13, 2023

/it

Copy link
Member

@o-liver o-liver 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

@o-liver
Copy link
Member

o-liver commented Oct 13, 2023

FYI @kaylinche @alxsap

@artembannikov
Copy link
Contributor Author

@o-liver Please trigger the pipelines again. I have provided fixes for failing unit tests.

@o-liver
Copy link
Member

o-liver commented Oct 13, 2023

/it

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@o-liver
Copy link
Member

o-liver commented Oct 13, 2023

/it

@o-liver o-liver enabled auto-merge (squash) October 13, 2023 13:44
@o-liver o-liver merged commit 09940e8 into SAP:master Oct 13, 2023
andrew-kireev pushed a commit that referenced this pull request Oct 17, 2023
…pload and export to node (#4624)

* Provide additional log message on successful upload and export to node

---------

Co-authored-by: Oliver Feldmann <oliver.feldmann@sap.com>
maxatsap pushed a commit to maxatsap/jenkins-library that referenced this pull request Jul 23, 2024
…pload and export to node (SAP#4624)

* Provide additional log message on successful upload and export to node

---------

Co-authored-by: Oliver Feldmann <oliver.feldmann@sap.com>
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