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

Support multiple projects per change #572

Closed
wants to merge 5 commits into from

Conversation

fsmaia
Copy link

@fsmaia fsmaia commented Nov 3, 2023

Closes #571

Check the following

  • Keep 100% code coverage
  • Be properly formatted
  • Documentation changes are included
  • Include a change file if expected

Additional context
Converting the project flag from StringVarP to StringSliceVarP on the new cmd kept the backward compatibility

@fsmaia fsmaia force-pushed the use-multi-select-on-projects branch 2 times, most recently from 648c72e to d0b3eb5 Compare November 3, 2023 11:07
@fsmaia fsmaia force-pushed the use-multi-select-on-projects branch from d0b3eb5 to 2574553 Compare November 3, 2023 11:39
@fsmaia fsmaia marked this pull request as draft November 3, 2023 11:46
@fsmaia fsmaia marked this pull request as ready for review November 3, 2023 11:59
@fsmaia fsmaia marked this pull request as draft November 3, 2023 12:04
@fsmaia
Copy link
Author

fsmaia commented Nov 3, 2023

So far I managed to update the new command properly, and now I'm dealing with an unexpected side effect on the batch command.

Scenario:

  1. Configure two projects on the config file
  2. Create a change (using new) that affects both projects
  3. Run batch command for the first project

Expected behavior: the change should remain unreleased on the second project

Actual behavior: the change is dropped from unreleased as soon as it is released by the first project

@fsmaia fsmaia force-pushed the use-multi-select-on-projects branch 2 times, most recently from 04f96e5 to 70587a5 Compare November 3, 2023 14:21
@fsmaia fsmaia marked this pull request as ready for review November 3, 2023 14:21
@fsmaia fsmaia force-pushed the use-multi-select-on-projects branch from 70587a5 to c6ffa4e Compare November 3, 2023 14:30
@fsmaia
Copy link
Author

fsmaia commented Nov 3, 2023

@miniscruff, I fixed the side effect on the batch command with the following behavior: when running the batch command for a project, the project entry will be removed from unreleased files. If the unreleased entry contains only one project, the unreleased file will be removed (because the change will be released for all the projects on it).

Because of this, I had to increase the complexity lint rule from 16 to 17, is that ok?
I couldn't find a way of refactoring without changing the batch command too much.

@fsmaia fsmaia force-pushed the use-multi-select-on-projects branch from 647dead to d91956d Compare November 3, 2023 14:33
@miniscruff
Copy link
Owner

@miniscruff, I fixed the side effect on the batch command with the following behavior: when running the batch command for a project, the project entry will be removed from unreleased files. If the unreleased entry contains only one project, the unreleased file will be removed (because the change will be released for all the projects on it).

Because of this, I had to increase the complexity lint rule from 16 to 17, is that ok?
I couldn't find a way of refactoring without changing the batch command too much.

Oh, yea I wouldn't change the change struct, I would create a separate file for each project. That should have the least issues with batching.

@fsmaia
Copy link
Author

fsmaia commented Nov 3, 2023

I prefer the implemented way for some reasons:

  • The unreleased change file gets more idiomatic and concise because it states that a change may have multiple projects, instead of duplicating all its content
  • The prompt belongs to a change (change.AskForPrompts), and refactoring the prompt to produce several changes would produce a huge change in the code

If you still prefer producing a change for each project, let me know, and then I can update the pull request.

@miniscruff
Copy link
Owner

I prefer the implemented way for some reasons:

  • The unreleased change file gets more idiomatic and concise because it states that a change may have multiple projects, instead of duplicating all its content
  • The prompt belongs to a change (change.AskForPrompts), and refactoring the prompt to produce several changes would produce a huge change in the code

If you still prefer producing a change for each project, let me know, and then I can update the pull request.

While I do agree with you, by keeping one file to one change per project it allows us to keep the only two operations as read the whole file and deleting it, no edits. Yes, it is maybe a little weird creating multiple files, but by keeping each project separate we can release them using PRs that for sure won't conflict. If we had to edit a file that would create an issue that changie aims to solve via files.

The internal yaml files, although can be changed, they should sort of be treated as hidden system files, so whether or not we create many or one file shouldn't be an issue.

@fsmaia
Copy link
Author

fsmaia commented Nov 3, 2023

Yep, I agree with the importance of having changes as atomic as possible.

Conceptually, the new command creates a change, which has validations and prompts within it.

For me, it's not clear how the New.Run should behave.

It seems to me that the unreleased file should have only one project, but I would keep a project list on the Change structure since it is a business rules structure, and not only the file representation.

@fsmaia
Copy link
Author

fsmaia commented Nov 3, 2023

To implement this, we should move the AskPrompts method to a new structure, and then we should collect the result of promptForProjects for generating multiple changes with the same configuration.

Do you see another way around this?

@fsmaia fsmaia force-pushed the use-multi-select-on-projects branch 2 times, most recently from 46fbf5e to 2e0cc61 Compare November 3, 2023 19:45
@fsmaia fsmaia force-pushed the use-multi-select-on-projects branch from 2e0cc61 to 64fc448 Compare November 3, 2023 19:47
@fsmaia
Copy link
Author

fsmaia commented Nov 3, 2023

I just managed a way to move the prompt to another structure that accepts multiple projects, so far called PromptChange.
Could you check if the implementation is okay?

PS.: some tests are still broken, I'll be able to fix them on Monday.

@miniscruff miniscruff self-requested a review November 3, 2023 19:50
@@ -1,6 +1,7 @@
package cmd
Copy link
Owner

Choose a reason for hiding this comment

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

The code in this PR doesn't seem wrong, I will have to try it out myself to be sure. But it is quite a bit of changes and I just want to give it a try myself before merging. I will try and get to it this weekend, I may create my own PR or edit this one, not sure yet.

One thing is your change from change to prompt.go is a fairly large change and I want to make sure I fully understand it. Thanks for the work though this is a big help for sure.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, besides being a big change, I don't think that the "prompt change" is the correct abstraction, btw it's hard to find the proper naming for the layer between the new command and the atomic change itself.

Personally I would try to avoid moving all of it to the cmd package.

Definitely, feel free to update this PR or open a new one, I'm still here if any help is needed!

Thank you!!

@fsmaia
Copy link
Author

fsmaia commented Nov 9, 2023

@miniscruff any news on this?

@miniscruff
Copy link
Owner

@miniscruff any news on this?

Sorry I am away on a trip this week, still on my list of todos.

@miniscruff
Copy link
Owner

Ok, going to tackle this today, finally able to get time to figure this out.

@miniscruff
Copy link
Owner

Closing in favor of #579, I did take a bit of inspiration from your work with a few tweaks.
Thanks again @fsmaia

@miniscruff miniscruff closed this Nov 12, 2023
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.

Support multiple projects in a single change
2 participants