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

advisory: move command #1042

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

Dentrax
Copy link
Member

@Dentrax Dentrax commented Jul 8, 2024

Move a package's advisories into a new package.

This command will move most advisories for the given package into a new package. And rename the
package to the new package name. (i.e., from foo.advisories.yaml to foo-X.Y.advisories.yaml) If the
target file already exists, the command will try to merge the advisories. To ensure the advisories
are up-to-date, the command will start a scan for the new package.

This command is also useful to start version streaming for an existing package that has not been
version streamed before. Especially that requires manual intervention to move the advisories.

The command will move the latest event for each advisory, and will update the timestamp
of the event to now. The command will not copy events of type "detection", "fixed",
"analysis_not_planned", or "fix_not_planned".


Test it:

1. `go install .`
2. Jump to https://github.com/wolfi-dev/advisories/ dir
3. Run `wolfictl advisory stream kyverno kyverno-1.12`

@Dentrax Dentrax marked this pull request as ready for review July 9, 2024 15:05
@luhring
Copy link
Contributor

luhring commented Jul 10, 2024

Thanks for doing this, @Dentrax! This looks like it will be useful.

One overall question before diving in too deep: would this be more useful as a generalized "move"/"rename" command? It sounds like one use case is when we start to version stream a package that wasn't version streamed before, like the kyverno example. But IIUC, the same logic applies for any package rename — does that sound right to you?

I'm wondering if by generalizing this command to something like wolfictl advisory mv (inspired by and analogous to the Unix mv command, which handles file renames too), and keeping almost all of the logic in this PR (minus the version stream regex checking), we'd solve not just the "new version stream" case but all package rename cases... and also be well on our way to a "moved across repos" solution as well. What are your thoughts? Feel free to tell me I'm misunderstanding things here. 😄

@Dentrax Dentrax changed the title advisory: stream command advisory: move command Jul 10, 2024
@Dentrax
Copy link
Member Author

Dentrax commented Jul 10, 2024

Great advice @luhring! Renamed to move (mv). 👍

@luhring
Copy link
Contributor

luhring commented Jul 10, 2024

Would you mind looking at the code a bit again before we review further? It looks like there is still some version stream logic left over, documentation files from the old command name, and references to scanning that we ended up deciding not to do. Once this is in a state that's ready for more scrutiny, just ping me!

@Dentrax Dentrax force-pushed the advisory-stream branch 2 times, most recently from dac327d to 4b7e0a2 Compare July 11, 2024 16:05
Signed-off-by: Dentrax <[email protected]>
@Dentrax
Copy link
Member Author

Dentrax commented Jul 11, 2024

Apologies for the oversight @luhring! I should have double-check it.

It's now ready for review. PTAL when possible!

and references to scanning that we ended up deciding not to do.

I couldn't find this, actually i didn't include this functionality. 🤔

Comment on lines +95 to +100
evts = []v2.Event{evts[len(evts)-1]}

// Update the timestamp to now.
evts[0].Timestamp = v2.Now()

advisory.Events = evts
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This feels a little awkward to me. Could we capture the event we want, then do the timestamp update, and then just return a slice that includes that event? (It seems strange to operate on the event within the slice when we don't need to)

This command will move most advisories for the given package into a new package. And rename the
package to the new package name. (i.e., from foo.advisories.yaml to foo-X.Y.advisories.yaml) If the
target file already exists, the command will try to merge the advisories. To ensure the advisories
are up-to-date, the command will start a scan for the new package.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this PR still needs some cleanup from our last discussion?

err := index.Remove(filename)
require.NoError(t, err)

assert.NotContains(t, index.paths, filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this line for? It looks like we're testing the removal in the test block below?

assert.NotContains(t, index.paths, filename)
})

t.Run("ensure the config is removed", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also test that other configs weren't removed and are still available?

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