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

Update Notify package management #658

Closed
6 tasks done
jzbahrai opened this issue May 18, 2022 · 33 comments
Closed
6 tasks done

Update Notify package management #658

jzbahrai opened this issue May 18, 2022 · 33 comments
Assignees
Labels
Dev Task for implementation of a technical solution Medium Priority | Priorité moyenne Refined l Affiné Stories and tasks that are ready to be worked on Security | Sécurité Tech Debt An issue targeting an identified technical debt

Comments

@jzbahrai
Copy link
Collaborator

jzbahrai commented May 18, 2022

Description

We can backport some changes from GDS and their improved way of doing package management.

CDS documentation on patch management: https://github.com/cds-snc/platform-sre-security-support/wiki/Patch-management

Please look at this thread for discussion:
https://gcdigital.slack.com/archives/CV38DBNVA/p1652794882038499

GDS has introduced some improvements by using pip-tools and pyup that we can also replicate. These would allow for transitive dependencies traces.

There is also poetry which could an option instead of pip-tools / pyup. To evaluate and consider.

Acceptance Criteria

Explore in the ADR the following items:

  • ADR to see how our tools overlap or integrate and explore alternatives (likely pip-tools or poetry) to our requirements-app.txt / make freeze-requirements approach.
  • Move to Poetry.
  • Using Poetry to maintain upgrades and keep track of versioning automatically.
  • Check with @mohamed-cds on renovate and how we want to integrate. Don't do the integration now but lay out the future plan and cards necessary to make a migration from snyk to renovate.

QA

  • The Notify API, admin, document-download API (and maybe utils) make use of the updated package management tooling.
  • A dependency bump was performed using the new tooling on each component.

Additional Context

The current Notify setup makes use of a Make target named freeze-requirements. This is an in-house GDS target to resolve transitive dependencies based on an intake and desired list of dependencies. The intake currently sits as requirements-app.txt file while the resolved dependencies will get resolved as the requirements.txt file, sitting at the root of the project.

With the new changes in, the dependency intake would actually be in a pyproject.toml file instead and the resolved dependencies are stored in the poetry.lock file.

The pyup tool most likely would not be adopted as we have similar tooling (Renovate) that detects vulnerabilities and automatically opens PRs with potential fixes and Renovate natively supports Poetry. We might want to check if that offers anything new... but also, this might conflict with new solutions being brought in by the SRE team (ask @mohamed-cds ).

The Poetry tool seems quite mature and we instill best practices.

@yaelberger-commits yaelberger-commits changed the title Introduce a Package Manager ADR to explore status quo and a best practice Package Manager May 18, 2022
@yaelberger-commits
Copy link
Collaborator

@yaelberger-commits yaelberger-commits changed the title ADR to explore status quo and a best practice Package Manager ADR to explore status quo and a best practice Package Manager (GDS practices) May 18, 2022
@jimleroyer jimleroyer changed the title ADR to explore status quo and a best practice Package Manager (GDS practices) ADR to explore status quo and a best practice Package Manager May 18, 2022
@jimleroyer jimleroyer changed the title ADR to explore status quo and a best practice Package Manager ADR to explore status quo for package management and version bump May 18, 2022
@jimleroyer
Copy link
Member

@sastels and @andrewleith Are you OK if we move forward without an ADR and implement the suggestions listed in the acceptance criteria?

@sastels
Copy link
Contributor

sastels commented Jun 15, 2022

Sounds reasonable, especially if GDS has had success. If it ends up not working then we can explore alternative approaches through an ADR.

@andrewleith
Copy link
Member

I agree!

@jimleroyer jimleroyer changed the title ADR to explore status quo for package management and version bump Update Notify package management and version bump check Jun 15, 2022
@jimleroyer
Copy link
Member

Sounds good then! @sastels and @andrewleith , if you can estimate on the issue then as-is, not for the ADR but the actual changes, it would be much appreciated. 🙏

@yaelberger-commits yaelberger-commits added High Priority | Haute priorité Tech Debt An issue targeting an identified technical debt labels Jul 4, 2022
@yaelberger-commits yaelberger-commits added the Refined l Affiné Stories and tasks that are ready to be worked on label Jul 4, 2022
@yaelberger-commits
Copy link
Collaborator

please also see link in description to CDS documentation for patch management

@mohdnr
Copy link

mohdnr commented Jul 15, 2022

If you want to see the new patch management setup in action take a look at cds-snc/scan-websites#322. This is a dashboard that the new tool "renovate" will maintain to help you keep track of what needs to be patched.

How to add it to you repo (my team can help):

  1. Give the Renovate GitHub app permission on the repo; and
  2. Submit a PR to the repo to add the Renovate config. This uses a default config that was setup for all of CDS.

Here’s the spot in the GitHub app that you add repos:
image

@jimleroyer jimleroyer changed the title Update Notify package management and version bump check Update Notify package management Jul 25, 2022
@jzbahrai jzbahrai removed their assignment Aug 18, 2022
@andrewleith
Copy link
Member

Just a note that we are meeting with SRE this week re:renovate which will hopefully take care of at least a portion of this work.

@patheard
Copy link
Member

I did some research on Renovate's support for transitive dependency trace comments and it doesn't provide this. However, it is able to consume a pip-compile requirements.in dependency file using the following config:

{
  "pip-compile": {
    "fileMatch": ["(^|/)requirements\\.in$"]
  }
}

@yaelberger-commits
Copy link
Collaborator

Some overlap with Renovate dependency tracker

@yaelberger-commits yaelberger-commits added the Dev Task for implementation of a technical solution label Dec 23, 2022
@whabanks whabanks self-assigned this Jan 5, 2023
@whabanks
Copy link

whabanks commented Jan 9, 2023

I threw together a quick doc on Pip-tools vs Poetry based on my findings thus far.

@yaelberger-commits
Copy link
Collaborator

Could also look into Renovate as well

@whabanks
Copy link

Regarding the question: Can we leverage Renovate in place of Poetry / in conjunction with pip instead of moving to Poetry?

Renovate has little to no overlap with package managers. It provides DevOps oriented tooling, that notifies us via PR's with suggested version updates for dependencies.

Renovate does this by reading package files generated by package managers like poetry, pip, pip-tools or npm and searches PyPi to check if deps listed in the package file are out of date.

All this to say: Renovate cannot do what tools like pip or Poetry do for us and depends on the usage of a package manager in a project to fulfill its function. We can use these tools in conjunction with one another but neither is a replacement for the other.

@yaelberger-commits
Copy link
Collaborator

yaelberger-commits commented Jan 18, 2023

Small draft PR with Poetry for anyone who wants to play around with it

@whabanks
Copy link

Updated the Pip-tools vs Poetry doc with some comparisons between migrating projects to both Pip-tools and Poetry. Added a short blurb on the level of risk associated with migrating to each tool.

@yaelberger-commits
Copy link
Collaborator

yaelberger-commits commented Jan 23, 2023

Will working today, doing some work on github actions workflows from pip to poetry. Start migrating other projects over to Poetry as well

@andrewleith
Copy link
Member

  • API is in a stable state; github actions now working; some makefile cleanup needed
  • ADMIN is in progress
  • DD-API is next

@andrewleith
Copy link
Member

  • Admin/dd-api/api are all using Poetry; github actions updated as well
  • @whabanks working on utils today

@yaelberger-commits
Copy link
Collaborator

Document download API PR is ready to go
Utils API admin still some work to clean up, should be ready today for review

@smcmurtry
Copy link
Contributor

  • all PRs are up on each repo for the migration
  • @jimleroyer reviewed the PR on API, will merge and test that one today before moving on to the others

@YedidaZalik
Copy link

Document download API successfully deployed, running with Poetry
Small problem with docker file
Working on possible solutions and hoping for 1 deployment

@smcmurtry
Copy link
Contributor

@whabanks could you add an update to this card?

@smcmurtry
Copy link
Contributor

Yesterday successfully deployed Admin changes to staging. Hoping to get the notification-api changes deployed today.

@smcmurtry
Copy link
Contributor

API ready for another test deployment to staging. Pat identified a compatibility issue with Renovate causing errors. Will troubleshooting that today.

@whabanks
Copy link

Moving to blocked:
Renovate is currently broken for Admin and is expected to be broken in API as well. It looks like until we can self host Renovate to increase the execution timeout this issue will persist. This is just a soft block, other work such as deploying and testing API and Utils can still continue.

@smcmurtry
Copy link
Contributor

Issues with the lambda API image, looks like we may need to self-host renovate to make it work with Poetry.

@sastels
Copy link
Contributor

sastels commented Feb 23, 2023

lambda api issues fixed, poetry PR merged to staging and tested.

@yaelberger-commits
Copy link
Collaborator

Relevant Slack thread for current state
https://gcdigital.slack.com/archives/CV38DBNVA/p1677075866073539

@andrewleith
Copy link
Member

  • Still having issues
  • Looking to trying to self-host renovate to get around memory issues

@patheard
Copy link
Member

The problem is that the werkzeug update to 2.2.3 is causing a dependency conflict since notification-utils is pinned to werkzeug==2.2.2. This then triggers an infinite loop in Poetry:

I chatted with @whabanks and I'm going to submit a Renovate config change to ignore werkzeug updates to see if this is the only dependency conflict we're hitting.

Ideally there will be an upstream fix to Poetry that allows it to fail gracefully on conflicts.

@patheard
Copy link
Member

patheard commented Feb 27, 2023

After having Werkzeug ignored by Renovate, things are working again!
image

Until Poetry resolves the bug, we'll have to periodically check that Renovate is completing since unfortunately the temporary-error on the Renovate side doesn't get surfaced so it's easy to miss that it's happening.

@andrewleith
Copy link
Member

Great news!

Will other package bumps that both admin and utils depend on lead to this situation? There are several that the two projects have that need to stay in sync when doing dep bumps.

@patheard
Copy link
Member

patheard commented Mar 1, 2023

Yup, it's possible that we'll hit this problem with other products if you're using Poetry to manage the dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev Task for implementation of a technical solution Medium Priority | Priorité moyenne Refined l Affiné Stories and tasks that are ready to be worked on Security | Sécurité Tech Debt An issue targeting an identified technical debt
Projects
None yet
Development

No branches or pull requests

10 participants