Skip to content

Conversation

olivierdalang
Copy link
Collaborator

@olivierdalang olivierdalang commented Aug 4, 2022

Dependencies

TODO

  • Fix more inconsistencies highlighted by latest version (>0.9.14) of pum

Notes

I think the tests do work as expected, and highlighted the following issues (which are fixed in this PR):

  • the qwat_sigip schema is present in the releases, but does not seem supported by the deltas. For now, I'm just manually dropping the schema in the CI before running pum check. But this schema should be removed from the dumps, or the deltas fixed to work with it.
  • tables related to documents were missing in the initialization scripts
  • pre/pos-all scripts could silently fail (because of os.system that does not raise exceptions on failure)
  • a minor inconsistency in the name of an index
  • a script that was not compatible with postgres<=10 (which should still be supported)
  • this does not yet include anything about making release (which still uses the same travis script that at also tests upgrades with pum check). Removing this overlap and fixing the release (e.g. remove sigip extension) will be done in a separate PR.

@olivierdalang olivierdalang changed the title [WIP][CI] include pum check in CI to test datamodel upgrades [WIP-DO NOT MERGE][CI] include pum check in CI to test datamodel upgrades Aug 4, 2022
(not too sure how pum check could work for such optional
components... IMO we should make them non-optional)
@olivierdalang olivierdalang changed the title [WIP-DO NOT MERGE][CI] include pum check in CI to test datamodel upgrades [CI] include pum check in CI to test datamodel upgrades Aug 12, 2022
@olivierdalang olivierdalang marked this pull request as ready for review August 12, 2022 10:13
@ponceta
Copy link
Member

ponceta commented Aug 30, 2022

#363 Should fix qwat_sigip related issues. If not I will be glad to contribute to fix it properly to ensure small customizations are supported.

@ponceta
Copy link
Member

ponceta commented Aug 30, 2022

@olivierdalang review asked to oslandia if that's ok for you?

@olivierdalang
Copy link
Collaborator Author

@ponceta Yes sure :-)

@ponceta ponceta requested a review from lbartoletti November 7, 2022 07:01
Copy link
Collaborator

@lbartoletti lbartoletti left a comment

Choose a reason for hiding this comment

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

LGTM, with previous comments and one for a gnu-ism

while [[ $# > 0 ]]; do
key="$1"
case $key in
-h|--help)
Copy link
Collaborator

Choose a reason for hiding this comment

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

bash builtin long option names with the double-dash prefix. This is an extension from GNU getopt (on macos and bsd-like you have to install gnu-getopt).

More details: #228
and from Denis https://stackoverflow.com/a/37485578 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I copied this from tests.sh, thus I suggest to keep it like that for consistency

(to be honest, no idea what this is about :-D)

@olivierdalang
Copy link
Collaborator Author

I think this can be merged provided we're aware of this risk:

I'm not 100% sure this wont cause havoc for those who are not using the latest pum version, since there is a change to migrations files.

For the change in update/delta/delta_1.2.8_schema_visible_perf.pre.py, I guess pum doesn't keep track of its checksum anyway (it's a pre migration), so I think it would not cause issues. If we revert that, we may have silently failing issues.

For the changes in 1.4.0_*, since not release yet, I guess we can consider they are in no one's production database. If we revert that, we loose postgres 9.6 compatibility.

@ponceta ponceta reopened this Nov 11, 2022
@ponceta ponceta merged commit eefdb9c into master Nov 11, 2022
@ponceta ponceta deleted the test-pum-upgrade branch November 11, 2022 05:49
@ponceta
Copy link
Member

ponceta commented Nov 11, 2022

Thank you @olivierdalang ! We will fix the remaining bug and stuff when they appear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants