Remove duplicate image.upload fedmsg on AMI upload completion #97
Conversation
sinnykumari
force-pushed
the
develop
branch
2 times, most recently
from
1b66287 to
a4aa7f4
Compare
There was a problem hiding this comment.
How do you know that nobody is depending on this message? Since this is a backwards incompatible change, it might be best to make this change with a major version release of fedimg, and should also get release notes that highlight that the message will no longer be sent.
|
@bowlofeggs I agree with you that it's possible to miss out some projects who maybe relying on image.upload message. So far we know that fedora-website uses this message to show Atomic and Cloud-base AMIs information. We will ensure that website team is aware of this change and right patches get applied on time to keep website stable. As per my understanding, after the re-write/refactoring of fedimg with 1.0, image.upload message signifies that upload of cloud image file has been done in aws with snapshot created successfully. There is another message sent by fedimg with topic image.publish which tells that AMIs are publicly available in aws region(s). So, it makes sense to me that if an application is relying on fedimg messages to get Fedora AMIs available to use then it should look for image.publish message and not image.upload. Correct me @sayanchowdhury if I said something wrong :) I understand that if we want to maintain compatibility then we shouldn't merge this PR. But it makes sense to me to have this change in fedimg and definitely communicate in next release notes properly about this incompatible change. |
|
@bowlofeggs Makes sense. fedimg uses Semantic Versioning so the next release would get a major bump. |
|
To be clear, I'm not suggesting we shouldn't merge this PR, but I do think it needs release notes added to it to make this clear. +1 to giving it a major version bump too :) I'd be willing to give this PR a +1 if it can include some release notes that highlight that the message is gone. It would also be good to send a note to fedora-infra and probably the releng list to let them know as well, just in case something is relying on this fedmsg. Not sure where else might want to know, maybe QA? |
|
@sinnykumari Can you rebase this branch? |
We already send image.upload fedmsg notification in EC2ImageUploader::_register_image() method on an AMI upload completion. Sending again same notification on AMI getting published shouldn't be required. Signed-off-by: Sinny Kumari <[email protected]>
|
rebase done |
We already send image.upload fedmsg notification in
EC2ImageUploader::_register_image() method on an AMI
upload completion. Sending again same notification
on AMI getting published shouldn't be required.
Signed-off-by: Sinny Kumari [email protected]