Output customizations: REPORT_TITLE and OUTPUT_FILE

[spier]

Proposed Changes

Contributes to #156.

My plan is to implement customizability for both REPORT_TITLE and OUTPUT_FILE in this PR.

Status:

• REPORT_TITLE - First working implementation of a customizable REPORT_TITLE
• OUTPUT_FILE - Not implemented yet

I am sharing this PR early, to get feedback about preferred implementation and testability

Readiness Checklist

Author/Contributor

• If documentation is needed for this change, has that been included in this pull request
• run make lint and fix any issues that you have introduced
• run make test and ensure you have test coverage for the lines you are introducing

Reviewer

• Label as either bug, documentation, enhancement, infrastructure, or breaking

[spier]

@zkoppert I am adding some questions in inline comments, where I wasn't sure how to best implement things.

[spier]

While writing the inline comments I think i convinced myself that I should implement the default value for the REPORT_TITLE at a different place. Looking forward to feedback though :)

[spier]

@zkoppert I applied the suggested changes. That means moving the default value for the report title into the write_to_markdown() signature. It does lead to a better code structure overall, I think.

However one issue remains in this area:
We feed in the custom title as a parameter here.
Therefore we are not actually testing if it is passed through from the REPORT_TITLE env variable correctly.

Btw the same issue exists in the testing logic for the hide_label_metrics in row 305 above.
There I am not sure either if this test really covers what it should.

[spier]

I think overall I am a bit unclear about the intended logic of using ENV vars in the code.

Some ENV vars are read in various functions in issue_metrics.py - via main(), get_env_vars(), and others.
Yet again other ENV vars are read directly in markdown_writer.py.

[zkoppert]

Ideally we would consolidate all env variables being read from the get_env_vars() function.

[zkoppert]

For this PR it would be good to follow that pattern. We can open a separate PR to get everything using the proper get_env_vars() function.

[spier]

@zkoppert I found these traces in the documentation.

Apparently at some point in the past, a file object was passed to the write_to_markdown().
Do you happen to know anything about this?

As I am looking to add the feature to set a custom OUTPUT_FILE, I don't want to go down an implementation path that might have been already tried in the past (and did not work). :)

[spier]

Adding a link to the commit where I removed that unused bit of documentation. GitHub did not give me a straightforward way to get there from my comment above.
5c5cd84

[zkoppert]

Yeah I think that was aspirational. I don't think we actually wrote the code to support that.

[spier]

@zkoppert just had a somewhat random idea:

What if we would implement the same functionality but with fewer ENV variables?

Idea

We could make the GHA append to issue_metrics.md, rather than to create a new file each time.
Then the users could run the report generation steps in the order, in which we want the reports to appear in the final GitHub issue.

That way we would not have to introduce any new ENV variables.

However, this would likely be a breaking change, for anybody that is already creating combined reports today.

Alternative

OR, we add only a single variable COMBINE_REPORTS = True, which when set will do the appending as described above. This way we might not create a breaking change, if the default for COMBINE_REPORTS was False?

Not 100% clear about the pros/cons but figured I just share it for input :)

[zkoppert]

@zkoppert just had a somewhat random idea:

What if we would implement the same functionality but with fewer ENV variables?

Idea

We could make the GHA append to issue_metrics.md, rather than to create a new file each time. Then the users could run the report generation steps in the order, in which we want the reports to appear in the final GitHub issue.

That way we would not have to introduce any new ENV variables.

However, this would likely be a breaking change, for anybody that is already creating combined reports today.

Alternative

OR, we add only a single variable COMBINE_REPORTS = True, which when set will do the appending as described above. This way we might not create a breaking change, if the default for COMBINE_REPORTS was False?

Not 100% clear about the pros/cons but figured I just share it for input :)

Because the alternative is a breaking change, I prefer the route of COMBINE_REPORTS = True.

[jmeridth]

This has been open for a while. Completing in #373 and adding @spier and @zkoppert as co-authors. Once that merges, this will close. There are a few notes in the thread above that may turn into new issues (i.e., COMBINED_REPORTS = True).