feat: allow passing substitution params as objects

[ijakab]

Ability to pass substitution params to update method. Converts key-value pairs to substitution params when creating command line arguments:

-D {propertyKet}=${propertyValue}

[vuki656]

Any updates on this? @tabuckner

[vuki656]

Hey, @tabuckner would really love it if we can revisit this.

What do you think about adding params in the initialization like so:

const myConfig: LiquibaseConfig = {
    ...POSTGRESQL_DEFAULT_CONFIG,
    url: 'jdbc:postgresql://localhost:5432/node_liquibase_testing',
    username: 'yourusername',
    password: 'yoursecurepassword',
    params: {
        key: "value"
    }
}

const instance = new Liquibase(myConfig)

[MalloD12]

Hi @tabuckner, is there any update on this? Should we move forward with this PR as it is, or do you think we can implement parameters addition as proposed by @vuki656?

Thanks, Daniel.

[MalloD12]

Hey @nvoxland, do you think we can move forward with this? Are you able to review this PR?

Thanks, Daniel.

[tabuckner]

@MalloD12 , looking in to this now

[tabuckner]

One initial question I have is: "What other commands should be able to accept parameter substitution?" If these are all of them, then I think this sort of approach only needs some minor changes to make it a bit more flexible.

However, if there are other commands that should be able to accept the parameter substitution, then we'd want to ensure those methods are updated as well.

[tabuckner]

@ijakab @Spolja @MalloD12 I'm not sure if this PR flow makes the most sense, but I just opened a New PR against this branch on @ijakab's fork.

In the PR I outline some of the changes that I feel would be a little more maintainable going forward, and also do some small cleanup/refactor.

If we're all in agreement, we could merge https://github.com/ijakab/node-liquibase/pull/1 into @ijakab's fork, and then ensure that #45 has the same code. If it does and everything looks okay, we could merge into master, but might need to redeploy. Someone would need to take a look at the semantic release setup to make sure everything is going as planned. I wish I could help more, but it's been a while since I looked at the CI/CD pipeline for this project.

Alternatively, I can open another PR directly against liqubase:master, but @ijakab may not get the contribution credits.

[filipelautert]

Thanks @tabuckner ! Let's see if @ijakab haven't gone MIA, otherwise a new PR from you would be welcome.

@ijakab are you fine with this approach (merge https://github.com/ijakab/node-liquibase/pull/1 into your fork and then we merge this PR)?

[kevin-atx]

@ijakab - were you able to merge https://github.com/ijakab/node-liquibase/pull/1 into your fork? Are we cleared to merge this PR?

[ijakab]

Hi @kevin-atx, wow I did not expect the interes on this. I did merge it on a fork somewhere, but never refactored it to have some cleaner API - this was the easiest solution that worked and did not pay much mind to it. However, for the purporses of the library, I think it is better to decline this PR and go with the cleaner API (e.g. have some kind of .withSubstitutionParams method or pass it through the constructor.

But if you want to merge this PR - I can confirm it caused no issues, it will just be a weird API and all the public methods will need to accept this param.

[kevin-atx]

Thanks for the quick response, @ijakab. I will forward this PR to our internal review team and see how they want to proceed.

[jegsar]

Just another dev interested in seeing this functionality added, willing to contribute if anything is needed.

[tabuckner]

I was able to open a new PR against node-liquibase using the branch in https://github.com/ijakab/node-liquibase/pull/1 here: https://github.com/liquibase/node-liquibase/pull/75

I didn't test the PR at all, but it should at least allow for the changed code, a combination of https://github.com/liquibase/node-liquibase/pull/45 and the slight refactor I did in https://github.com/ijakab/node-liquibase/pull/1 directly into the node-liquibase repo.

The current destination branch of this new PR is master, so that might need to be changed for testing before releasing. It's been a while since I checked out the CI/CD setup for node-liquibase, but I think there would be an automatic deploy of some sort.

Hopefully this helps unblock the new code, and move things forward a bit 🙏

EDIT: Just noticed that this newest PR is against my own fork. 🤦 . I'll try to re-open against the Liquibase fork.

EDIT 2: Added PR to Liquibase fork.

[kevin-atx]

Hey @tabuckner - I'm going to add this PR to the sprint that starts this Thursday so we can get it merged into main and release a long-needed update to this extension. Since we don't have a lot of node expertise, do you have any guidance for @filipelautert when he's reviewing this?

[filipelautert]

Implemented on https://github.com/liquibase/node-liquibase/pull/75 . Any issues please reopen this PR.