Package not pushed to myget after merged PR

naacbin commented 1 month ago

What's the problem?

Both PR merge 2145 and 2148 have a CI that has failed, meaning that no packages have been uploaded to MyGet. For the first one it's due to the hash of IDA that was incorrect at the moment of the merge. Even if IDA is now fixed (#1060) other packages in the PR haven't been pushed. For the second one, it's because npcap.com site was and is still down (fyi nmap.org is down too),. Once they are back again, a rerun of the job should be enough.

Steps to Reproduce

N/A

Environment

N/A

Additional Information

No response

Detected by test suite

No

naacbin commented 4 weeks ago

I have re-run the CI job for 2148, the packages from #1052 are now available on MyGet. However, for the CI of #1053 (2145, packages must be uploaded manually to MyGet, as the hash of IDA has changed the CI will always fail. @Ana06

Ana06 commented 3 weeks ago

Thanks for letting me know @naacbin! We should push the packages and then ensure that this does not happen again (for example changing the CI to push the packages that do work or/and addressing https://github.com/mandiant/VM-Packages/issues/132).

If I understand correctly, we need to push all packages in https://github.com/mandiant/VM-Packages/pull/1053. I'll try to do this manually later today. Alternatively editing the packages time in the nuspec version and merging that PR would push them as well.

naacbin commented 3 weeks ago

If I understand correctly, we need to push all packages in #1053. I'll try to do this manually later today. Alternatively editing the packages time in the nuspec version and merging that PR would push them as well.

Yes, all except IDA.

Ana06 commented 3 weeks ago

Sorry for the late reply @naacbin, this is important, but I have had some busy days.

After taking a closer look to this, I think:

We should try to avoid updating many packages in the same PR (if the update is not related) as it makes reviewing the changes easier/faster and prevents failures like the one in this issue. In general atomic changes are a good idea. I'll document this.
We should ensure the CI in PRs run in the same day before merging it (specially if many packages are updated at the same time). I'll document this.
We should address https://github.com/mandiant/VM-Packages/issues/132 to ensure we notice the issue if it happens again. I am working on it right now.
We should push the missing packages using a PR where we increase the version to push the packages using the same procedure as usual. I will send 2 PRs for this.

I considered modifying the GH actions to push the packages that succeed even when others in the same PR fail when merging. But this can have unexpected effects if the changes in the packages are related (what is often the case for changes in the same PR). It doesn't happens often (it is the first time that I remember), so I think we can decide how to handle and handle it manually if it happens again.

Does this sound good to you @naacbin @mandiant/vms ?

naacbin commented 3 weeks ago

Sorry for the late reply @naacbin, this is important, but I have had some busy days.

Same for me no, worries.

Does this sound good to you @naacbin @mandiant/vms ?

A little bit too late but LGTM !

Thanks for all the recent merge @Ana06

Ana06 commented 3 weeks ago

@naacbin I have added the promised documentation to https://github.com/mandiant/VM-Packages/wiki/Coding-Conventions#small-pull-requests could you check it out and let me know if you have any feedback (either in https://github.com/mandiant/VM-Packages/issues/728 or opening a new issue as you see it fit)?

mandiant / VM-Packages