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

Fix --clean flag in federated generator #1475

Merged
merged 6 commits into from
Jan 15, 2023
Merged

Conversation

axmmisaka
Copy link
Collaborator

@axmmisaka axmmisaka commented Nov 17, 2022

This hopefully completes the task mentioned in #1221.

This PR did......

  1. Added FedGenerator::cleanIfNeeded() just like other generators; 86681d6#diff-e0f660764015b35c4b0bf3bb21720b1f7be727746c3790043b30139de8f90671R174
  2. Override FedFileConfig::doClean(), 86681d6#diff-aef6a300a7e1bac3cddb7efb7226ba4b7e59333e269617fd75d591baaad0a5e3R85

So in total, it will clean ./src-gen (which will not exist if federated), ./bin (appears to exist regardless of federated) and ./fed-gen/{name} (unlike in non-federated context, removing the whole directory makes sense as the whole directory is generated).

@lhstrh mentioned to me that one more thing to do is to tell each federate to do --clean; I'm unsure if that is been done or not now as during federate generation context which possibly contains clean flag will be passed in:

Marking this WIP until I confirm with people the expected behaviour of --clean under federated: would cleaning these 3 places be enough

As the whole directory is generated it makes sense to remove all of them.
@axmmisaka axmmisaka changed the title [WIP] Fix --clean flag in federated generator Fix --clean flag in federated generator Nov 18, 2022
@axmmisaka axmmisaka marked this pull request as ready for review November 18, 2022 19:15
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Minor change requested. Otherwise looks good!

@petervdonovan
Copy link
Collaborator

The requested change has been made, and I am in such a hurry to get fed-gen merged that I will take the liberty of merging this branch into it. If any lingering issues remain, we can deal with them later, maybe in the fed-gen branch.

@petervdonovan petervdonovan merged commit 0ff948f into fed-gen Jan 15, 2023
@petervdonovan petervdonovan deleted the axmmisaka/fix-clean branch January 15, 2023 06:55
@lhstrh
Copy link
Member

lhstrh commented Jan 15, 2023

I think it's a good decision. Thanks for being so proactive, @petervdonovan!

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.

3 participants