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

Dependabot updates for eslint, djlint, uswds, and postcss-cli #1715

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

danswick
Copy link
Contributor

@danswick danswick commented Aug 3, 2023

Per chat with @asteel-gsa, merging dependabot PRs is causing deploy issues. I went ahead and updated a few of the node dependencies to help clear the backlog out.

eslint: #1650
postcss-cli: #1630
uswds: #1629
djlint: #1627

There's also stylelint, but it seems to have a conflicting peer dependency.

@danswick danswick temporarily deployed to dev August 3, 2023 22:08 — with GitHub Actions Inactive
@danswick danswick temporarily deployed to management August 3, 2023 22:08 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Terraform plan for management

No changes. Your infrastructure matches the configuration.
No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration
and found no differences, so no changes are needed.

✅ Plan applied in Deploy to Development and Management Environment #70

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Terraform plan for dev

No changes. Your infrastructure matches the configuration.
No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration
and found no differences, so no changes are needed.

✅ Plan applied in Deploy to Development and Management Environment #70

@danswick danswick marked this pull request as ready for review August 3, 2023 22:09
Copy link
Contributor

@jadudm jadudm left a comment

Choose a reason for hiding this comment

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

I don't know what our process will be for these.

Ultimately, our staged deploy process will answer a lot of questions:

  1. Will the PR commit/deploy to main fail? If so, that will block staging.
  2. Will the daily to staging fail? If so, we'll know.

So, really, the updates on versions to appease our robotic overlords (I, for one, welcome them) seem like we should test locally, and bring them through. I don't know that we have another way to test.

@asteel-gsa , if you think there's a better way, give a shout.

@jadudm jadudm merged commit 14d88e4 into main Aug 4, 2023
@jadudm jadudm deleted the ds-node-dependabots branch August 4, 2023 09:46
@asteel-gsa
Copy link
Contributor

@danswick thanks for grabbing these updates, much appreciated.

@jadudm I brought this up in one of the dev huddles when stylelint was just the problem. RE my findings yesterday, since dependabot forks, it can't actually grab our secrets and run anything, even if it wanted. (we can enable this, but it is a rather large security concern 🤣)

So... we realistically have two options.

  • We bump the versions, draft a pr. Thankfully, linting and baseline testing when a PR is created, catches most surface level issues. Bumping github action related items is fairly non trivial. There is a given backwards compatibility, BUT it is good to read the update notes. Basically for GHA it comes down to.... was a core piece of the functionality removed. If so, and we use it, we cant update.
  • As for the deeper app stack dependencies, In my experience, there is some degree of backwards compatibility, but the main item to address is if we version bump, can we build the stack. I put in this nifty little check so if anything gets changed in requirements.txt, dev-requirements.txt (via pip-compile from .in files), dockerfile or package.json (which is what dan modified here)
Results:
Filter requirements = false
Filter devrequirements = false
Filter docker = false
Filter package = true
Changes output set to ["package"]

it is going to build the container from scratch and then run tests with the new dependencies. This is a fairly safe "test" for these version bumps. Can we build the stack, and if we can, can we run our tests against it with those new versions. That is effectively what a dev would do, if they were doing this locally.

Basically, what I am trying to say, is that I added this safeguard, thanks to some insight from @neilmb as a means to offset a developers need to do all of this locally.

We do gain confidence if it builds in the workflow, passes, and a dev has validated that these bumps don't actually break anything from their perspective on the local stack, but.. from an automation perspective, we have a safety net for these events.

Now. Dependabot creating PRs, is more of a visual reminder that we need to upgrade our dependencies. I for one don't regularly check the dependabot security tab for version bumps, but having PRs out there saying these versions are out of date, helps me, and the fact that our dependabot.yml tracks npm, docker, terraform and github action updates means that most of the core stack is accounted for in terms of PRs that are visible. What isn't tracked, is pip requirements because when Bret and I were looking at this, it directly modifies requirements.txt which.. we don't want. We looked for a way to do requirements.in but didn't see anything that would benefit us. Those, will still be tracked in the security tab, and should have a local build and test done, to ensure everything is passing.

@mogul
Copy link
Contributor

mogul commented Aug 4, 2023

We looked for a way to do requirements.in but didn't see anything that would benefit us.

The answer I found later was to move our dependencies list into pyproject.toml! That's the new consensus for a tooling-neutral Python manifest, and critical Python tooling now supports it. So Dependabot will make PRs there if it finds one.

We would need someone who understands the implications on local and CI workflows better than me to make that change, though, so I was holding off on suggesting we do that ahead of other stuff on our plate that those people are already occupied with.

@mogul
Copy link
Contributor

mogul commented Aug 4, 2023

That's the new consensus for a tooling-neutral Python manifest, and critical Python tooling now supports it. So Dependabot will make PRs there is it finds one.

Here's a summary with links and related discussion that I wrote up in another context.

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.

4 participants