Skip to content

Add MySQL and Postgres port precedence tests; refactor environment variable checks#1872

Open
shaunagm wants to merge 5 commits into
mainfrom
check-env-refactor
Open

Add MySQL and Postgres port precedence tests; refactor environment variable checks#1872
shaunagm wants to merge 5 commits into
mainfrom
check-env-refactor

Conversation

@shaunagm
Copy link
Copy Markdown
Collaborator

@shaunagm shaunagm commented May 22, 2026

What is this change?

  • Introduced tests for MySQL and Postgres to verify that the MYSQL_PORT and PGPORT environment variables correctly override default and explicit port settings.
  • Refactored database connection classes to utilize a new check_env utility for environment variable retrieval, enhancing error handling and code clarity.
  • Updated relevant imports across multiple files to support the new environment variable checking mechanism.

Considerations for discussion

Likely some merge conflicts with #1826. I can wait for that PR to merge and resolve conflicts before merging.

Breaking Changes

Does this PR introduce breaking changes?
  • label: Breaking change — This PR introduces one or more breaking changes.
  • label: Non-breaking change — This PR does not introduce one or more breaking changes.

Details (if needed)

Technically the change in how port defaults/parameters is resolved is a breaking change. Previously, PGPORT/MYSQL_PORT env variables were effectively ignored when port wasn’t passed, because the port parameter defaults to an integer which evaluates as truthy. So port or os.environ.get("PGPORT") always used 5432 even when no parameter was passed and a PG_PORT env variable was set.

If anyone was relying on the default port number passed into the mysql or postgres connectors overriding an environmental variable, then they would experience a breaking change. Although it seems pretty odd that someone would set an environmental variable but rely on it being overridden? So I'm not too concerned.

But will ship this with other breaking changes and name it in the release notes just to be cautious.

EDIT: I changed my approach to be less of a breaking change. But it is still a very very slight breaking change.

…riable checks

- Introduced tests for MySQL and Postgres to verify that the MYSQL_PORT and PGPORT environment variables correctly override default and explicit port settings.
- Refactored database connection classes to utilize a new `check_env` utility for environment variable retrieval, enhancing error handling and code clarity.
- Updated relevant imports across multiple files to support the new environment variable checking mechanism.
@github-actions
Copy link
Copy Markdown

ghost commented May 22, 2026

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  parsons/aws
  s3.py 32-35
  parsons/azure
  azure_blob_storage.py 52-55
  parsons/box
  box.py 67-70
  parsons/databases/mysql
  mysql.py
  parsons/databases/postgres
  postgres.py
  parsons/databases/redshift
  redshift.py
  parsons/facebook_ads
  facebook_ads.py 83-89
  parsons/mobilize_america
  ma.py
  parsons/notifications
  slack.py
  parsons/salesforce
  salesforce.py
Project Total  

This report was generated by python-coverage-comment-action

@bmos
Copy link
Copy Markdown
Collaborator

bmos commented May 23, 2026

Feel free to merge before #1826, any conflicts should be minimal.
This is great!

@bmos
Copy link
Copy Markdown
Collaborator

bmos commented May 23, 2026

Import sorting is a ruff check feature, for whatever reason.

@shaunagm
Copy link
Copy Markdown
Collaborator Author

Updated my approach. It bothered me that someone passing in the default port would have it overridden, so I set the default port to None and put the default in the init code itself.

So now:

  • if you don't pass in anything or set an env variable, the default is still used
  • if you don't pass in anything but do set an env variable, the env variable is used
  • if you pass in something (whether it's identical to the default or not) that gets used

Still technically a breaking change but it's so hard to imagine why someone would have their system set up in a way that this would break it so.

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

Labels

breaking-change applied only to PRs, indicates when something has a breaking change to be flagged in release notes python Pull requests that update Python code testing Work type - writing or changing code tests for core Parsons features or Parsons connectors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants