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

Write changelog only if changes exist #5164

Merged

Conversation

brachi-wernick
Copy link
Contributor

Impact

  • [ x] Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Write changelog only if changes exist.

When running diffChangeLog and there are no changes at all, it still generates an empty changelog file.
It makes a lot of garbage file that needed to be removed manually.

Signed-off-by: Brachi Packter <brachipackter@gmail.com>
@MalloD12 MalloD12 changed the base branch from master to 3.10.x November 10, 2023 19:55
@MalloD12 MalloD12 changed the base branch from 3.10.x to master November 10, 2023 19:55
@MalloD12 MalloD12 added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label Nov 10, 2023
@MalloD12
Copy link
Contributor

MalloD12 commented Nov 14, 2023

Created DAT-16261 to fix failing functional tests.

@suryaaki2 suryaaki2 self-requested a review December 7, 2023 19:54
@rberezen rberezen added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Dec 7, 2023
@MalloD12 MalloD12 added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Dec 7, 2023
@MalloD12
Copy link
Contributor

MalloD12 commented Dec 8, 2023

@rberezen could you please let me know when DAT-16261 is done, so I can approve this PR when all checks are green?

Thanks,
Daniel.

@obovsunivskyii
Copy link
Contributor

obovsunivskyii commented Dec 8, 2023

hi @brachi-wernick Thanks for adding this. Even when we generate a new diff changelog with no differences, we're still receiving this message.

Liquibase command 'diffChangelog' was executed successfully.

I think it would be nice to trigger a warning message in such scenarios, as

WARNING: No differences have been detected. The changelog [name of the changelog] was not generated.

CC: @MalloD12

@brachipa
Copy link

@obovsunivskyii @MalloD12
This message is part of generic logging of Liquibase command and not speifically to diffChange log.

Liquibase command 'diffChangelog' was executed successfully.

If you check the logs you will see some information about empty changes:

changeSets count: 0
No changesets to add.
Diff changelog command succeeded
Command execution complete
Liquibase command 'diff-changelog' was executed successfully.

I Think better to change the log

No changesets to add.

to be as the warning you suggested

WARNING: No differences have been detected. The changelog [name of the changelog] was not generated.

And this will be in the scope of class DiffToChangeLog. and no need to start passing information between the change log class and the generic Liquibase classes.

Let me know if it sounds good and if so I'll commit it.

@brachipa
Copy link

brachipa commented Dec 31, 2023

I'm same person as brachi-wernick, this is just my git work user (by mistake, forgot to switch)

@MalloD12
Copy link
Contributor

MalloD12 commented Jan 2, 2024

@obovsunivskyii @MalloD12 This message is part of generic logging of Liquibase command and not speifically to diffChange log.

Liquibase command 'diffChangelog' was executed successfully.

If you check the logs you will see some information about empty changes:

changeSets count: 0
No changesets to add.
Diff changelog command succeeded
Command execution complete
Liquibase command 'diff-changelog' was executed successfully.

I Think better to change the log

No changesets to add.

to be as the warning you suggested

WARNING: No differences have been detected. The changelog [name of the changelog] was not generated.

And this will be in the scope of class DiffToChangeLog. and no need to start passing information between the change log class and the generic Liquibase classes.

Let me know if it sounds good and if so I'll commit it.

Hi @brachipa,

I'm fine with it, but what do you guys think if we lightly change that message to something like:

No changesets to add to the changelog output.

How does it sound to you? cc: @obovsunivskyii

Thanks,
Daniel.

@obovsunivskyii
Copy link
Contributor

@obovsunivskyii @MalloD12 This message is part of generic logging of Liquibase command and not speifically to diffChange log.
Liquibase command 'diffChangelog' was executed successfully.
If you check the logs you will see some information about empty changes:

changeSets count: 0
No changesets to add.
Diff changelog command succeeded
Command execution complete
Liquibase command 'diff-changelog' was executed successfully.

I Think better to change the log
No changesets to add.
to be as the warning you suggested
WARNING: No differences have been detected. The changelog [name of the changelog] was not generated.
And this will be in the scope of class DiffToChangeLog. and no need to start passing information between the change log class and the generic Liquibase classes.
Let me know if it sounds good and if so I'll commit it.

Hi @brachipa,

I'm fine with it, but what do you guys think if we lightly change that message to something like:

No changesets to add to the changelog output.

How does it sound to you? cc: @obovsunivskyii

Thanks, Daniel.

I'm okay with these changes @MalloD12 @brachi-wernick thank you

fix message as discussed

Signed-off-by: Brachi Packter <brachipackter@gmail.com>
@rberezen
Copy link
Contributor

rberezen commented Jan 8, 2024

Looks good to me. Functional tests have been fixed in https://github.com/liquibase/liquibase-pro-tests/pull/1176. Thanks, @brachi-wernick, for your contribution!

@suryaaki2
Copy link
Contributor

@filipelautert This shouldn't cause any breaking change for the customers, right?

@filipelautert
Copy link
Collaborator

@filipelautert This shouldn't cause any breaking change for the customers, right?

@suryaaki2 Right! This PR skips the creation of a file that would just have xml headers on it.

Copy link
Contributor

@MalloD12 MalloD12 left a comment

Choose a reason for hiding this comment

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

Approved.

Code changes looks good to me, and failing tests have been fixed in this PR. Thank you, guys (@brachi-wernick - @rberezen)!!

@MalloD12
Copy link
Contributor

MalloD12 commented Jan 9, 2024

@filipelautert I think we can already merge this one since @rberezen has already created the PR to fix the failing functional tests, and also merge Ruslan's PR.

Thanks,
Daniel.

@filipelautert filipelautert added this to the 1NEXT milestone Jan 9, 2024
@filipelautert filipelautert merged commit 8743812 into liquibase:master Jan 9, 2024
29 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions TypeEnhancement
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants