Skip to content

Conversation

@tjmoore4
Copy link
Contributor

Checklist:

  • Have you added an explanation of what your changes do and why you'd like them to be included?
  • Have you updated or added documentation for the change, as applicable?
  • Have you tested your changes on all related environments with successful results, as applicable?
    • Have you added automated tests?

Type of Changes:

  • New feature
  • Bug fix
  • Documentation
  • Testing enhancement
  • Other

What is the current behavior (link to any open issues here)?

What is the new behavior (if this is a feature change)?

  • Breaking change (fix or feature that would cause existing functionality to change)

Other Information:

@tjmoore4 tjmoore4 force-pushed the pgadmin-9.3-updates branch 4 times, most recently from ed573dd to c9283eb Compare September 25, 2025 20:03
Changes in the flags used by pgAdmin's setup.py for user managment start in pgAdmin 9.3. Issue: PGO-2686
Capture the an expected user warning for pgAdmin9.8 using python3.11 and log as an INFO message rather than an ERROR which short-circuits user creation and updating.
@tjmoore4 tjmoore4 marked this pull request as ready for review September 26, 2025 18:12
Copy link
Member

@cbandy cbandy left a comment

Choose a reason for hiding this comment

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

I only had time for a quick look so far.

script := fmt.Sprintf(`
PGADMIN_DIR=%s
cd $PGADMIN_DIR && python3 -c "import config; print(config.APP_RELEASE)"
cd $PGADMIN_DIR && python3 -c "import config; print(config.APP_VERSION)"
Copy link
Member

Choose a reason for hiding this comment

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

🤔 It looks like the result of this is only used when reconciling users. Can we move this into the commands there?

📝 It looks like pgAdmin 8.4 and newer have a version module with just these constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into whether moving the commands makes sense. My guess is that would probably be fine as a refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. The previous functions been removed and one test added to account for the scenario that wasn't being tested.

}

var olderThan9_3 bool
versionFloat, err := strconv.ParseFloat(pgadmin.Status.MinorVersion, 32)
Copy link
Contributor

@benjaminjb benjaminjb Sep 29, 2025

Choose a reason for hiding this comment

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

why bitsize 32 here but 64 when we retrieve the number to put into the status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Short version: That can probably be a 64, I'll make that change.

Longer version: I originally used 32 because I was intending to store the value directly into the status as a float. However, that's counter-indicated due to issues in float implementation and a string value is recommended instead. When the conversion happens, that function allows for 32 to be set so that the result will be convertible to float32 without changing its value despite returning a type float64 and, at the time, I wanted to use float32 because float64 was overkill for a simple version value.

@tjmoore4 tjmoore4 merged commit e395e2a into CrunchyData:main Sep 29, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants