FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

WIP: chore(deps): upgrade pgx to v5 by janisz · Pull Request #6915 · stackrox/stackrox · GitHub

/ stackrox Public

WIP: chore(deps): upgrade pgx to v5 - #6915

Closed
janisz wants to merge 1 commit into
masterfrom
master-janisz/07-12-chore_deps_upgrade_pgx_to_v5
ConversationCommits (1)ChecksFiles changed
Closed

WIP: chore(deps): upgrade pgx to v5#6915
janisz wants to merge 1 commit into
masterfrom
master-janisz/07-12-chore_deps_upgrade_pgx_to_v5

Conversation

janisz commented Jul 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

pgx follows semantic versioning for the documented public API on stable releases.
v5 is the latest stable major version.

Important changes in v5:

Checklist

  • Investigated and inspected CI test results
  • Unit test and regression tests added
  • Evaluated and added CHANGELOG entry if required
  • Determined and documented upgrade steps
  • Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

CI

janisz requested a review from a team July 12, 2023 19:58
janisz requested review from a team as code owners July 12, 2023 19:58

ghost commented Jul 12, 2023 •
edited by ghost
Loading

Copy link
Copy Markdown

Images are ready for the commit at 44009eb.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.1.x-541-g44009eb6f0.

janisz force-pushed the master-janisz/07-12-chore_deps_upgrade_pgx_to_v5 branch from 447de3e to e765e3b Compare July 12, 2023 20:25
stehessel requested changes Jul 12, 2023
View reviewed changes

stehessel left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think you need some more fixes in the unit tests because of this change:

CommandTag is now an opaque type instead of directly exposing an underlying []byte.

See for example

=== RUN   TestAlertDataStore/TestConnExec
    conn_test.go:106: 
        	Error Trace:	/__w/stackrox/stackrox/pkg/postgres/tests/conn_test.go:106
        	Error:      	Expected nil, but got: pgconn.CommandTag{s:""}
        	Test:       	TestAlertDataStore/TestConnExec
    conn_test.go:112: 
        	Error Trace:	/__w/stackrox/stackrox/pkg/postgres/tests/conn_test.go:112
        	Error:      	Expected nil, but got: pgconn.CommandTag{s:""}
        	Test:       	TestAlertDataStore/TestConnExec

janisz commented Jul 13, 2023

Copy link
Copy Markdown
Contributor Author

Indeed the handling or arrays also changed causing problems https://github.com/jackc/pgx/blob/v5.4.2/CHANGELOG.md?plain=1

janisz force-pushed the master-janisz/07-10-ROX-18155_pg_generic_store_Upsert branch from f9f38c6 to 8f100c9 Compare July 13, 2023 10:32
janisz force-pushed the master-janisz/07-12-chore_deps_upgrade_pgx_to_v5 branch from e765e3b to 3152b48 Compare July 13, 2023 10:32
This was referenced Jul 13, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
janisz force-pushed the master-janisz/07-12-chore_deps_upgrade_pgx_to_v5 branch from 3152b48 to 30917ac Compare July 13, 2023 11:32

janisz commented Jul 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

janisz mentioned this pull request Jul 13, 2023
Merged
5 tasks
janisz mentioned this pull request Jul 14, 2023
Closed
5 tasks

dashrews78 commented Jul 14, 2023

Copy link
Copy Markdown
Collaborator

I haven't had a chance to look at this in detail yet, but I do have a question. What level of testing have we done on this? Reason I ask is that this looks like a significant upgrade and we've seen some things in the past where pgx took some liberties with how they do things. My concern is they adjusted some behaviors and we won't necessarily see them with just vanilla CI. Things we found during scale and chaos type testing would be missed in CI.

Additionally should we go through the v5 changelog and write tickets to step up to some of their improved flows such as the new way batching works, etc?

stehessel commented Jul 14, 2023

Copy link
Copy Markdown
Collaborator

I haven't had a chance to look at this in detail yet, but I do have a question. What level of testing have we done on this? Reason I ask is that this looks like a significant upgrade and we've seen some things in the past where pgx took some liberties with how they do things. My concern is they adjusted some behaviors and we won't necessarily see them with just vanilla CI. Things we found during scale and chaos type testing would be missed in CI.

Agreed, my review was mostly just looking at the changes surface level, but I don't believe we should bump this just because the unit tests pass. I don't know how these upgrades have been done in the past, but I'd be more comfortable with some larger scale tests on the new version.

janisz commented Jul 14, 2023

Copy link
Copy Markdown
Contributor Author

I concur we need some better test plan. So far I only manually verified some generated queries and besides of map nil serialization I haven't seen any differences. But queries are just a small part of what pgx does. I can enable run UI tests as they run more gql queries.

Unfortunately there is no upgrade guide for pgx. I was under the impression it's mostly backward compatible. E.g.: gorm just merged it and released as bugfix release and we already are using this version.

Maybe better time to merge it will be just after the release so we will have more time to catch possible bugs. We will need to bump it at some point.

janisz commented Jul 14, 2023

Copy link
Copy Markdown
Contributor Author

@connorgorman @mtodor could you help with perf/scale tests?

janisz commented Jul 17, 2023

Copy link
Copy Markdown
Contributor Author

/retest

janisz marked this pull request as draft July 18, 2023 10:48
janisz force-pushed the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch from da0fb0b to baad7ef Compare July 24, 2023 16:20
janisz force-pushed the master-janisz/07-12-chore_deps_upgrade_pgx_to_v5 branch from bbf53e7 to 0edb329 Compare July 24, 2023 16:20
janisz force-pushed the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch from baad7ef to f688304 Compare July 25, 2023 10:45
janisz mentioned this pull request Jul 25, 2023
Merged
5 tasks
janisz force-pushed the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch from f688304 to 7ccf79d Compare July 25, 2023 16:19
janisz mentioned this pull request Jul 25, 2023
Merged
1 task
janisz force-pushed the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch from 7ccf79d to 7d1d2f6 Compare July 25, 2023 18:27
Base automatically changed from master-janisz/07-13-refactor_pg_store_empty_map_as_not_null to master July 26, 2023 09:15
janisz force-pushed the master-janisz/07-12-chore_deps_upgrade_pgx_to_v5 branch from 0edb329 to 44009eb Compare July 26, 2023 15:31

janisz commented Jul 26, 2023

Copy link
Copy Markdown
Contributor Author

/test all

janisz marked this pull request as ready for review July 26, 2023 15:35
janisz changed the title chore(deps): upgrade pgx to v5 WIP: chore(deps): upgrade pgx to v5 Jul 26, 2023

janisz commented Jul 26, 2023

Copy link
Copy Markdown
Contributor Author

/test all

1 similar comment

janisz commented Jul 26, 2023

Copy link
Copy Markdown
Contributor Author

/test all

janisz commented Jul 27, 2023

Copy link
Copy Markdown
Contributor Author

/retest

openshift-ci Bot commented Jul 27, 2023

Copy link
Copy Markdown

@janisz: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ocp-4-10-compliance-e2e-tests 44009eb link false /test ocp-4-10-compliance-e2e-tests
ci/prow/gke-upgrade-tests 44009eb link false /test gke-upgrade-tests
ci/prow/ocp-4-10-qa-e2e-tests 44009eb link false /test ocp-4-10-qa-e2e-tests
ci/prow/ocp-4-13-core-bpf-qa-e2e-tests 44009eb link false /test ocp-4-13-core-bpf-qa-e2e-tests
ci/prow/ocp-4-13-qa-e2e-tests 44009eb link false /test ocp-4-13-qa-e2e-tests

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

janisz commented Jul 31, 2023

Copy link
Copy Markdown
Contributor Author

Closing, we can reopen it once we have a proper test plan for this upgrade.

janisz closed this Jul 31, 2023
janisz deleted the master-janisz/07-12-chore_deps_upgrade_pgx_to_v5 branch September 16, 2025 09:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

stehessel stehessel requested changes
connorgorman Awaiting requested review from connorgorman

Assignees

No one assigned

Labels

area/central area/postgres do-not-merge/work-in-progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL