Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updated requirements #1438

Merged
merged 6 commits into from
Dec 20, 2023
Merged

updated requirements #1438

merged 6 commits into from
Dec 20, 2023

Conversation

abroddrick
Copy link
Contributor

@abroddrick abroddrick commented Dec 5, 2023

Ticket

updated dependencies

Changes

  • update the requirements.txt and piplock file by following the instructions in docs/operations/runbooks/update_python_dependencies.md

Context for reviewers

a few updates got through without both these files being updated so this is to make sure all requirements are up to date.

Setup

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

  • Met the acceptance criteria, or will meet them in a subsequent PR
  • Created/modified automated tests
  • Added at least 2 developers as PR reviewers (only 1 will need to approve)
  • Messaged on Slack or in standup to notify the team that a PR is ready for review
  • Changes to “how we do things” are documented in READMEs and or onboarding guide
  • If any model was updated to modify/add/delete columns, makemigrations was ran and the assoicated migrations file has been commited.

Ensured code standards are met (Original Developer)

  • All new functions and methods are commented using plain language
  • Did dependency updates in Pipfile also get changed in requirements.txt?
  • Interactions with external systems are wrapped in try/except
  • Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

  • New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
  • Checked keyboard navigability
  • Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
  • Add at least 1 designer as PR reviewer

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

  • Pulled this branch locally and tested it
  • Reviewed this code and left comments
  • Checked that all code is adequately covered by tests
  • Made it clear which comments need to be addressed before this work is merged
  • If any model was updated to modify/add/delete columns, makemigrations was ran and the assoicated migrations file has been commited.

Ensured code standards are met (Code reviewer)

  • All new functions and methods are commented using plain language
  • Interactions with external systems are wrapped in try/except
  • Error handling exists for unusual or missing values
  • (Rarely needed) Did dependency updates in Pipfile also get changed in requirements.txt?

Validated user-facing changes as a developer

  • New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing

  • Checked keyboard navigability

  • Meets all designs and user flows provided by design/product

  • Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)

  • Tested with multiple browsers, the suggestion is to use ones that the developer didn't (check off which ones were used)

    • Chrome
    • Microsoft Edge
    • FireFox
    • Safari
  • (Rarely needed) Tested as both an analyst and applicant user

Note: Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist

As a designer reviewer, I have

Verified that the changes match the design intention

  • Checked that the design translated visually
  • Checked behavior
  • Checked different states (empty, one, some, error)
  • Checked for landmarks, page heading structure, and links
  • Tried to break the intended flow

Validated user-facing changes as a designer

  • Checked keyboard navigability

  • Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)

  • Tested with multiple browsers (check off which ones were used)

    • Chrome
    • Microsoft Edge
    • FireFox
    • Safari
  • (Rarely needed) Tested as both an analyst and applicant user

Screenshots

Copy link

github-actions bot commented Dec 5, 2023

🥳 Successfully deployed to developer sandbox ab.

geventconnpool@ git+https://github.com/rasky/geventconnpool.git@1bbb93a714a331a069adf27265fe582d9ba7ecd4
geventconnpool@ git+https://github.com/rasky/geventconnpool.git
Copy link
Contributor

@zandercymatics zandercymatics Dec 5, 2023

Choose a reason for hiding this comment

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

(Blocking) This will break the connection pool - it requires a specific commit number (of the latest commit). This library uses an old tag, so when you call this directly it grabs an old commit from 2014. Thats interesting that a pip update does that, maybe we need to discuss this in the next dev huddle?

This is one downside of that library but easily mitigated by forking it if we really need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have that documented somewhere? This then breaks our current setup with updating python requirements to this will need to be added into the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@abroddrick I was not aware of this issue, did you just follow the currently documented process to get this result?

Here is the only difference between fred / the gevent pool:
fred-epplib = {git = "https://github.com/cisagov/epplib.git", ref = "master"}
geventconnpool = {git = "https://github.com/rasky/geventconnpool.git", ref = "1bbb93a714a331a069adf27265fe582d9ba7ecd4"}

I wonder if this is simply a config thing, because it should be pointing towards a specific config number like it does for fred-epplib. I can include this in the documentation but we should pinpoint the underlying cause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documented this in our files for now, I think the Pipfile will need to be updated to reflect this somehow so it doesn't keep changing, but I don't want to hold this one up longer

Copy link

🥳 Successfully deployed to developer sandbox ab.

Copy link

🥳 Successfully deployed to developer sandbox ab.

Copy link

🥳 Successfully deployed to developer sandbox ab.

Copy link

🥳 Successfully deployed to developer sandbox ab.

Copy link

🥳 Successfully deployed to developer sandbox ab.

@abroddrick abroddrick dismissed zandercymatics’s stale review December 19, 2023 04:36

Changes made, re-review requested

Copy link

🥳 Successfully deployed to developer sandbox ab.

Copy link
Contributor

@zandercymatics zandercymatics left a comment

Choose a reason for hiding this comment

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

LGTM. Works locally and on sandboxes

geventconnpool@ git+https://github.com/rasky/geventconnpool.git@1bbb93a714a331a069adf27265fe582d9ba7ecd4
geventconnpool@ git+https://github.com/rasky/geventconnpool.git
Copy link
Contributor

Choose a reason for hiding this comment

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

@abroddrick I was not aware of this issue, did you just follow the currently documented process to get this result?

Here is the only difference between fred / the gevent pool:
fred-epplib = {git = "https://github.com/cisagov/epplib.git", ref = "master"}
geventconnpool = {git = "https://github.com/rasky/geventconnpool.git", ref = "1bbb93a714a331a069adf27265fe582d9ba7ecd4"}

I wonder if this is simply a config thing, because it should be pointing towards a specific config number like it does for fred-epplib. I can include this in the documentation but we should pinpoint the underlying cause

Comment on lines +16 to +19
3. Change geventconnpool back to what it was originally within the Pipfile.lock and requirements.txt.
This is done by either saving what it was originally or opening a PR and using that as a reference to undo changes to any mention of geventconnpool.
Geventconnpool, when set as a requirement without the reference portion, is defaulting to get a commit from 2014 which then breaks the code, as we want the newest version from them.
4. (optional) Run `docker-compose stop` and `docker-compose build` to build a new image for local development with the updated dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!!

@abroddrick abroddrick merged commit 31c0396 into main Dec 20, 2023
10 checks passed
@abroddrick abroddrick deleted the ab/requirements-update branch December 20, 2023 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants