Add support for multiple beans in the Flyway and Liquibase endpoints #6613

eddumelendez · 2016-08-11T04:28:33Z

wilkinsona · 2016-08-12T08:30:09Z

...ng-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/FlywayEndpoint.java

-		List<FlywayMigration> migrations = new ArrayList<FlywayMigration>();
-		for (MigrationInfo info : this.flyway.info().all()) {
-			migrations.add(new FlywayMigration(info));
+	public List<List<FlywayMigration>> invoke() {


Changing to List<List<FlywayMigration>> breaks backwards compatibility without any benefit. I think we should either keep the old format, or change to a Map<String, List> where the string somehow indicates which DataSource the migrations were run against. The JDBC URL of the DB would be ideal, but I'm not sure we can get that.

eddumelendez · 2016-08-16T05:05:12Z

Hi @wilkinsona if there any comments in this PR please let me know :)

wilkinsona · 2016-08-16T09:00:36Z

Thanks for the updates, @eddumelendez.

I wonder if we could enhance Boot's DataSourcePoolMetadata to provide access to the URL rather than having to establish a connection to the database. Please don't do anything about that just now as it might turn out to be a bad idea. I just wanted to make a note of it here so I didn't forget.

snicoll · 2016-10-10T13:17:21Z

I wonder if we could enhance Boot's DataSourcePoolMetadata to provide access to the UR

@wilkinsona I had a look to that and I am not sure we could, really. We would need to change the contract of DataSourcePoolMetadataProvider somehow. Sure we can abstract the url access in the implementation but that's something that would require to open a connection anyway.

snicoll · 2016-10-10T13:18:18Z

...ng-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/FlywayEndpoint.java


-	public FlywayEndpoint(Flyway flyway) {
+	public FlywayEndpoint(List<Flyway> flyway) {


You're breaking backward compat there. Can you please add the constructor with a single resource back and just call the other one with Collections.singletonList ?

snicoll · 2016-10-10T13:18:35Z

...ng-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/FlywayEndpoint.java

+		Map<String, List<FlywayMigration>> migrations = new HashMap<String, List<FlywayMigration>>();
+		for (Flyway flyway : this.flyway) {
+			try {
+				DatabaseMetaData metaData = flyway.getDataSource().getConnection().getMetaData();


You need to close the Connection.

snicoll · 2016-10-10T13:19:44Z

...boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/LiquibaseEndpoint.java

 			}
-			finally {
-				connection.close();


snicoll · 2016-10-10T13:20:38Z

...boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/LiquibaseEndpoint.java

-				return service.queryDatabaseChangeLogTable(database);
+				DatabaseMetaData metaData = liquibase.getDataSource().getConnection().getMetaData();
+				try {
+					DatabaseFactory factory = DatabaseFactory.getInstance();


Not sure you need to initialize the factory every time there.

snicoll · 2016-10-10T13:21:07Z

...test/java/org/springframework/boot/actuate/autoconfigure/EndpointAutoConfigurationTests.java

@@ -207,6 +212,20 @@ public void testFlywayEndpoint() {
 	}

 	@Test
+	public void testTwoFlywayEndpoint() {


typo, Endpoints

snicoll · 2016-10-10T13:21:58Z

...t/java/org/springframework/boot/actuate/autoconfigure/MvcEndpointPathConfigurationTests.java

@@ -153,12 +155,12 @@ public ConditionEvaluationReport conditionEvaluationReport(

 		@Bean
 		public FlywayEndpoint flyway() {
-			return new FlywayEndpoint(new Flyway());
+			return new FlywayEndpoint(Arrays.asList(new Flyway()));


Not breaking backward compat means you don't need to change this no more.

snicoll · 2016-11-03T11:31:34Z

@eddumelendez did you have a chance to review my comments? If you don't have time, let us know and we'll rework the PR.

eddumelendez · 2016-11-03T15:38:00Z

Sure @snicoll, I will update the PR. Thanks for feedback

See spring-projectsgh-6610

eddumelendez · 2016-11-04T02:56:35Z

@snicoll PR has been updated. Sorry, I just notice you reviewed this 25 days ago... looks like I didn't read that notification 😢

snicoll · 2016-11-04T09:32:42Z

Don't be, it could a different notification from Github or something. I've seen others not replying to that kind of requests.

snicoll · 2016-12-22T09:59:52Z

OK, that looks good except one thing I am not sure. The key is the URL of the database, so you end up with something like this:

{
  "jdbc:h2:mem:testdb": [
    {
      "type": "SQL",
      "checksum": 710039845,
      "version": "1",
      "description": "init",
      "script": "V1__init.sql",
      "state": "SUCCESS",
      "installedOn": 1482400309894,
      "executionTime": 6
    }
  ]
}

I see no other way to generate a sensible id for the datasource but perhaps the team has a better idea.

snicoll · 2016-12-22T10:28:06Z

Actually @bclozel helped me to answer that question.

Closes gh-6610 See gh-6613

* pr/6613: Polish contribution Add support for multiple beans in the Flyway and Liquibase endpoints

snicoll · 2016-12-22T10:47:18Z

Thank you @eddumelendez - Exposing the URL of the datasource is something we're not keen doing (the health endpoint doesn't do that). Besides, it's quite hard to "index" data based on the url.

For that stuff we usually use the name of the bean. That's what I've done in 5d909a9

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 11, 2016

philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 11, 2016

philwebb added this to the 1.5.0 milestone Aug 11, 2016

eddumelendez mentioned this pull request Aug 11, 2016

Support multiple beans in the Flyway and Liquibase endpoints #6610

Closed

wilkinsona reviewed Aug 12, 2016
View reviewed changes

eddumelendez force-pushed the gh-6610 branch from 3749c6a to 6df59f4 Compare August 13, 2016 12:06

philwebb modified the milestones: 1.5.0, 1.5.0 M1 Aug 30, 2016

snicoll modified the milestones: 1.5.0, 1.5.0 M1 Aug 31, 2016

snicoll requested changes Oct 10, 2016

View reviewed changes

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Oct 10, 2016

eddumelendez force-pushed the gh-6610 branch from 6df59f4 to b0195b4 Compare November 4, 2016 02:28

Add support for multiple beans in the Flyway and Liquibase endpoints

4abeea1

See spring-projectsgh-6610

eddumelendez force-pushed the gh-6610 branch from b0195b4 to 4abeea1 Compare November 4, 2016 02:29

eddumelendez removed the status: waiting-for-feedback We need additional information before we can continue label Nov 4, 2016

eddumelendez assigned snicoll Nov 4, 2016

snicoll removed their assignment Dec 1, 2016

snicoll approved these changes Dec 22, 2016

View reviewed changes

snicoll added the for: team-attention An issue we'd like other members of the team to review label Dec 22, 2016

snicoll self-assigned this Dec 22, 2016

snicoll removed the for: team-attention An issue we'd like other members of the team to review label Dec 22, 2016

snicoll pushed a commit that referenced this pull request Dec 22, 2016

Add support for multiple beans in the Flyway and Liquibase endpoints

53d2599

Closes gh-6610 See gh-6613

snicoll added a commit that referenced this pull request Dec 22, 2016

Merge pull request #6613 from eddumelendez:gh-6610

4b23528

* pr/6613: Polish contribution Add support for multiple beans in the Flyway and Liquibase endpoints

snicoll closed this in 5d909a9 Dec 22, 2016

joshiste mentioned this pull request Dec 27, 2016

Support multiple beans for Flyway and Liquibase codecentric/spring-boot-admin#371

Closed

eddumelendez deleted the gh-6610 branch January 17, 2018 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple beans in the Flyway and Liquibase endpoints #6613

Add support for multiple beans in the Flyway and Liquibase endpoints #6613

eddumelendez commented Aug 11, 2016

wilkinsona Aug 12, 2016 •

edited

eddumelendez commented Aug 16, 2016

wilkinsona commented Aug 16, 2016

snicoll commented Oct 10, 2016

snicoll Oct 10, 2016

snicoll Oct 10, 2016

snicoll Oct 10, 2016

snicoll Oct 10, 2016

snicoll Oct 10, 2016

snicoll Oct 10, 2016

snicoll commented Nov 3, 2016

eddumelendez commented Nov 3, 2016

eddumelendez commented Nov 4, 2016

snicoll commented Nov 4, 2016

snicoll commented Dec 22, 2016 •

edited

snicoll commented Dec 22, 2016

snicoll commented Dec 22, 2016


		public FlywayEndpoint(Flyway flyway) {
		public FlywayEndpoint(List<Flyway> flyway) {

Add support for multiple beans in the Flyway and Liquibase endpoints #6613

Add support for multiple beans in the Flyway and Liquibase endpoints #6613

Conversation

eddumelendez commented Aug 11, 2016

wilkinsona Aug 12, 2016 • edited

Choose a reason for hiding this comment

eddumelendez commented Aug 16, 2016

wilkinsona commented Aug 16, 2016

snicoll commented Oct 10, 2016

snicoll Oct 10, 2016

Choose a reason for hiding this comment

snicoll Oct 10, 2016

Choose a reason for hiding this comment

snicoll Oct 10, 2016

Choose a reason for hiding this comment

snicoll Oct 10, 2016

Choose a reason for hiding this comment

snicoll Oct 10, 2016

Choose a reason for hiding this comment

snicoll Oct 10, 2016

Choose a reason for hiding this comment

snicoll commented Nov 3, 2016

eddumelendez commented Nov 3, 2016

eddumelendez commented Nov 4, 2016

snicoll commented Nov 4, 2016

snicoll commented Dec 22, 2016 • edited

snicoll commented Dec 22, 2016

snicoll commented Dec 22, 2016

wilkinsona Aug 12, 2016 •

edited

snicoll commented Dec 22, 2016 •

edited