Added change summary and CVE report to state import
#3385
Conversation
There was a problem hiding this comment.
I'm sorry this is doing a lot for what the story is asking for. I'm ok with a bit of scope creep to avoid or address technical debt, but this is starting to refactor cve reporting. I'm not against refactoring, but we need to be more intentional with those kinds of jobs. I imagine a lot of churn going into this PR as is if we are to include refactoring, and we'd be going through that churn without having done any requirement setting for the refactor in the first place.
I see two options:
- Bring issue back to triage; we file a new refactor story as a blocker and we repurpose the code changes in this PR for the refactor.
- Undo the refactoring changes and just focus on the requirements for the story, then keep these changes for a follow refactor story.
My preference would be number 2, but I'll defer to you as there's obviously a reason you resorted to the refactor, and so this may be a hard blocker for addressing the story.
|
Okay, this PR now utilizes the new cves runbit from #3386. From the PR description, this PR
|
| rt, rtCommit, err := runtime.Solve(auth, out, i.prime.Analytics(), proj, &commitID, target.TriggerImport, i.prime.SvcModel(), i.prime.Config(), runtime.OptNone) | ||
| if err != nil { | ||
| return errs.Wrap(err, "Could not solve runtime") | ||
| } |
There was a problem hiding this comment.
This made me realize we're redundantly solving. Not asking for this to be addressed here, but for future context: https://activestatef.atlassian.net/browse/DX-2934
Filed it separately as this could easily get more complicated than just updating the model code and using the result from stageCommit directly (ie. UI implications).
internal/runners/packages/import.go
Outdated
| } | ||
| } | ||
|
|
||
| if err := localcommit.Set(proj.Dir(), commitID.String()); err != nil { |
There was a problem hiding this comment.
This should happen before the runtime update, otherwise runtime update failing will cause the import to have no effect. This makes sense for state install because you're only touching one thing, but for import I think you'd want to retain the broken state so you can fix it.
There was a problem hiding this comment.
Huh, that's the opposite of what I'd expect. If I do state import and my requirements.txt is causing a build failure, I'd want to edit my requirements.txt and try again, not do a bunch of state uninstall, state install operations to try and fix things.
That said, I'll defer to you, but I wanted to double-check this is what you want (to update the localcommit first, then update the runtime).
There was a problem hiding this comment.
Going a step further, it's like apt install <long list of deps> and committing even if there's a dependency failure, and then expecting you to apt uninstall and apt install your way back to a working system. Now I understand we have state reset which may sidestep this issue entirely, but it's still an extra step. With a big red 'x' saying my runtime failed to build, I think it's reasonable as a user to assume nothing was committed.
There was a problem hiding this comment.
@mitchell-as that's a good point, I'm honestly not sure. I could see an argument for both sides, because maybe the nature of the issue is not one that can be addressed by editing the requirements.txt, and further tweaking is required. In this case you'd need the interim state.
That said, the story didn't ask for a change to this mechanic, so I don't think we should be introducing it here. Especially when the value prop isn't black and white.
Also: