Skip to content

Conversation

eddumelendez
Copy link
Contributor

See gh-19200

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 6, 2019
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 6, 2019
@philwebb philwebb added this to the 2.3.x milestone Dec 6, 2019
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. While it technically does expose a property, I am not happy with the current documentation (and potentially how it is namespaced). I've added a question.

/**
* Name of the tag.
*/
private String tag;
Copy link
Member

Choose a reason for hiding this comment

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

While reviewing this PR, I was trying to improve the documentation of that property and couldn't really find my way in the liquibase documentation. The original issue states:

This property will help in generating roll back scripts for particular tags only when user has specified rollbackFile property.

Do you know more about this property and how it's actually used? I've looked into the liquibase source code and I am not sure I understood the scope of that property. If that's used only for rollbacks, I think we should make that clearer in the description of the property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when I was looking into the code I found SpringLiquibase uses tag property at performUpdate and generateRollbackFile methods. It means is executed during the update of changelogs or if additionally rollbackFile is also set will generate the file.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the feedback. Same but I think "Name of the tag" isn't enough for users to figure out what to do. Perhaps this is an issue in the Liquibase documentation?

Copy link
Member

Choose a reason for hiding this comment

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

I poked around and I am confused. I don't think we should just have a "tag" property, I've commented on the original issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for the late reply Stéphane! I have been thinking about it and TBH I think the workflow is pretty much deploying changes using the CLI and generating the rollback file from the application. As we have seen, the same property is used for rolling out and rollback. Said that, we should be very careful about the docs as you are worried about.

Delivering this feature, people will be able to:

  1. Use tag property, apply the tag value to the scripts

or

  1. Use tag property and rollbackFile, generate the file to perform a rollback

Copy link
Member

Choose a reason for hiding this comment

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

@SteveDonie thanks for the feedback and improving the docs.

We need a one sentence that would be a good summary for the property and we had a hard time finding something succinct.

Here is a proposal

Tag name to add to applied changes. Can be used with "rollbackFile" to generate a rollback script for all existing changes with that tag.

Thoughts?

Choose a reason for hiding this comment

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

I think that is pretty good. Maybe even call out that there are two different uses:

Tag name to apply when applying database changes. Tag name to use as a target when rolling back already applied changes.

Copy link

@SteveDonie SteveDonie Feb 14, 2020

Choose a reason for hiding this comment

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

When rolling back, you don't have to generate a rollbackFile. I'm not sure how it works in spring boot, but from the command line there are two commands - rollback, and rollbackSQL. One does the actual rollback, and the other generates the SQL to perform the rollback and shows it on the console (or sends it to a file).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks very much for the pointers, @SteveDonie.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, thank you @SteveDonie. FTR, Spring Boot "merely" instruments SpringLiquibase which is part of Liquibase itself.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Dec 23, 2019
@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Jan 14, 2020
@snicoll snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Jan 23, 2020
@wilkinsona wilkinsona added status: blocked An issue that's blocked on an external project change and removed for: team-attention An issue we'd like other members of the team to review labels Feb 13, 2020
@snicoll snicoll removed the status: blocked An issue that's blocked on an external project change label Feb 17, 2020
@snicoll snicoll self-assigned this Feb 17, 2020
@snicoll snicoll modified the milestones: 2.3.x, 2.3.0.M3 Feb 17, 2020
snicoll pushed a commit that referenced this pull request Feb 17, 2020
@snicoll snicoll closed this in d4c7315 Feb 17, 2020
@wilkinsona wilkinsona changed the title Add property tag for LiquibaseProperties Add support for configuration Liquibase's tag Mar 12, 2020
@wilkinsona wilkinsona changed the title Add support for configuration Liquibase's tag Add support for configuring Liquibase's tag Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants