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

Initial implementation of job summary #16

Merged
merged 5 commits into from Jun 1, 2022
Merged

Initial implementation of job summary #16

merged 5 commits into from Jun 1, 2022

Conversation

efasel
Copy link
Collaborator

@efasel efasel commented May 13, 2022

Inspired by …

… initial implementation, just to see that it's working. The report itself is still very basic.

As always: please feel free to criticise everything: style, shape, colour, etc 😄
Thanks!


private fun writeSuccessfulUpdates(updates: List<BuildpackUpdate>) {
file.appendText("\n## success\n\n")
updates.forEach { file.appendText("* currentBuildpack ${it.currentBuildpack} ${if (it.hasUpdate()) "has an update to " + it.latestUpdate else "has no update"}\n") }
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this monstrosity of a line? 🙈

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be tempted to move the if to a chained appendText, e.g.

updates.forEach {
  file.appendText("* currentBuildpack ${it.currentBuildpack} ")
  file.appendText(if (it.hasUpdate())"has an update to " + it.latestUpdate else "has no update")
  file.appendText("\n")
}

}

private fun writeSuccessfulUpdates(updates: List<BuildpackUpdate>) {
file.appendText("\n## success\n\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might need to be conditionally written, only when updates.length > 0? Otherwise you have a heading without content

}

private fun writeFailures(errors: Map<BuildpackUpdate, Exception>) {
file.appendText("\n## failures\n" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might need to be conditionally written, only when updates.length > 0? Otherwise you have a heading without content?

Also: newline-ception

Copy link
Collaborator

@jshiell jshiell 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, have some tweaks 😁

class SummaryWriter(val file: File) {

constructor(filename: String?) : this(
if (filename == null || filename.isBlank()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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


private fun writeSuccessfulUpdates(updates: List<BuildpackUpdate>) {
file.appendText("\n## success\n\n")
updates.forEach { file.appendText("* currentBuildpack ${it.currentBuildpack} ${if (it.hasUpdate()) "has an update to " + it.latestUpdate else "has no update"}\n") }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be tempted to move the if to a chained appendText, e.g.

updates.forEach {
  file.appendText("* currentBuildpack ${it.currentBuildpack} ")
  file.appendText(if (it.hasUpdate())"has an update to " + it.latestUpdate else "has no update")
  file.appendText("\n")
}


SummaryWriter(tempFile).write(successfulChecksResults)

tempFile.readText() shouldBeEqualTo "# CF buildpack update action results\n" + "\n" + "## success\n" + "\n" + "* currentBuildpack VersionedBuildpack(name=test/buildpack1, url=https://a.host/path, version=1.3, tag=GitTag(value=v1.3)) has no update\n" + "\n" + "Thanks for watching!\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can template the string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean raw strings? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

That works better tbh, much more readable

@efasel efasel merged commit 5e43467 into main Jun 1, 2022
@efasel efasel deleted the job_summary branch June 1, 2022 15:58
@efasel efasel restored the job_summary branch June 1, 2022 15:59
@efasel
Copy link
Collaborator Author

efasel commented Jun 1, 2022

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.

None yet

3 participants