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

merge request Review hermes post-processing results #205

Open
daniel-mohr opened this issue Aug 24, 2023 · 2 comments
Open

merge request Review hermes post-processing results #205

daniel-mohr opened this issue Aug 24, 2023 · 2 comments
Labels
5️ post-process The post-processing step in the workflow enhancement New feature or request help wanted Extra attention is needed question Further information is requested

Comments

@daniel-mohr
Copy link

Following the gitlab workflow described in Set up automatic software publishing leads to the merge request Review hermes post-processing results.

As mentioned in #202 and handled in softwarepub/ci-templates#7 a full virtual python environment is in the source branch hermes/post-... of the mentioned merge request. Therefore such a merge request cannot be handled! The important change in hermes.toml is hided in the 1662 new files. ;-)

Further I'm not sure if the change in hermes.toml is correct:

< from = [ "git", "cff" ]
---
> from = [ "git", "cff",]
7a8,10
> [postprocess]
> execute = [ "config_record_id",]
> 
11,13c14
< 
< [postprocess]
< execute = [ "config_record_id" ]
---
> record_id = ...
  • It is unnecessary to reorder the lines or to add senseless ,. This makes it more complicated for the user to see the relevant change.

  • Further I think calling config_record_id is not necessary anymore, is it?

And I did not expect that the uploaded zip archive is in the repo to merge.

@led02
Copy link
Member

led02 commented Sep 25, 2023

Well, config_record_id triggers the Post-Processor that adds the record_id key. In theorie, if this is successful we could in fact remove the according post-processor from the config.

However, I'm a bit hesitant here because:

  • The [postprocess] section is configured by hand. If a plug-in that is meant to "add record_id to the config" does remove the plugin from the post-processor list, this might lead to confusion.
  • While it would be fine for the plugin to remove itself from the execute list, it should not remove the whole section (as there might be other configurations and post-processors set up). Hence, we would need to add a 'clean up config' step (which might be a nice practice for providing additional post-processing plugins).

Regarding the format issues (i.e., trailing commas), this is due to the TOML library we use. We read in, modify, and write out again. If you know a better TOML library that does not introduce these additinal gutter, let us know.

@daniel-mohr
Copy link
Author

In my opinion the config_record_id and record_id are mutually exclusive. If the record_id is ones found and set, it is not necessary anymore to configure it. But I understand your point.

Regarding the format I can suggest TOML Kit: TOML Kit - Style-preserving TOML library for Python

But if you only add one key to a section, this could also be done without any library. But have to be implemented of cause. In this way it could be possible to use in future the python standard library tomllib -- available in python 3.11 and I expected newer versions -- which can only read toml.

@led02 led02 added enhancement New feature or request help wanted Extra attention is needed question Further information is requested 5️ post-process The post-processing step in the workflow labels Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5️ post-process The post-processing step in the workflow enhancement New feature or request help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants