Misc improvements to workflow documentation #816
Conversation
MichaelSimons
requested review from
mthalman and
oleksandr-didyk
as code owners
README.md
Outdated
|
|
||
| #### Workflow | ||
|
|
||
| * Generate reference package and its depencencies running the `./generate.sh --package <package>,<version>` script. | ||
| * Revert changes for packages that were already existed in the repository. | ||
| * Add `DependencyPackageProjects` for all new projects in the [eng/Build.props](https://github.com/dotnet/source-build-reference-packages/blob/main/eng/Build.props#L9) | ||
| * Revert any changes to packages that already existed in the repository. |
There was a problem hiding this comment.
I feel like we should have a test which validates the packages are up-to-date wrt the generator, specifically for GenAPI dependency flow PRs. It's really annoying to have to revert these changes.
There was a problem hiding this comment.
How often are you seeing changes that aren't intentional customizations now that everything has been regenerated with the genapi based tooling? Regenerating the code is not feasible at this point for two reasons.
- The tooling does not support re-generation scenarios today. There are some tracking issues that prevent this today.
- There are a number of intentional changes - e.g. explicit package upgrades to address CVEs as well as code modifications that genAPI does not support.
There was a problem hiding this comment.
I had to revert modifications to at least 2 packages I can remember for my changes in #815.
There was a problem hiding this comment.
Were they necessary changes that were manually applied to the packages to address a CVE or to get the code to compile or were they changes because genAPI was "improved"?
There was a problem hiding this comment.
I think it was just "improved".
There was a problem hiding this comment.
If you feel like regenerating the packages on dependency flow would improve the workflow, please log an issue for this.
README.md
Outdated
| * Revert changes for packages that were already existed in the repository. | ||
| * Add `DependencyPackageProjects` for all new projects in the [eng/Build.props](https://github.com/dotnet/source-build-reference-packages/blob/main/eng/Build.props#L9) | ||
| * Revert any changes to packages that already existed in the repository. | ||
| This is likely reverting a necessary source modification to the generated code. |
There was a problem hiding this comment.
This sentence seems at odds with the previous one. I think it will cause confusion. This makes it sound like it's a source modification that's needed but they should revert it -- which is confusing.
There was a problem hiding this comment.
Reworded to:
Inspect any changes to packages that already existed in the repository. There are two reasons why previously
generated packages show changes when being regenerated.
1. The package contains intentional code modifications on top of the generated code. This may be upgrading a
project reference to address a CVE or code fixups because the generate tooling does not support a scenario.
When this occurs, there should be code comments explaining why the code modification was made. If this is
the case, the changes to the existing package should be reverted.
2. The generate tooling has changed since the last time this package was generated. The new changes should
be considered better/correct and should be committed.
| 1. The package contains intentional code modifications on top of the generated code. This may be upgrading a | ||
| project reference to address a CVE or code fixups because the generate tooling does not support a scenario. | ||
| When this occurs, there should be code comments explaining why the code modification was made. If this is | ||
| the case, the changes to the existing package should be reverted. | ||
| 2. The generate tooling has changed since the last time this package was generated. The new changes should | ||
| be considered better/correct and should be committed. |
There was a problem hiding this comment.
This may end up being difficult for newbies to differentiate. We should strive to add comments to the source whenever making manual modifications so they are easy to spot.
There was a problem hiding this comment.
Agreed. Hopefully the last step I added will help ensure comments are added.
I broke these changes into two commits. The first one contains the content modifications. The second one is just shortening some long lines.