I'd prefer we can fetch real data of modules from pdc while running in dry_run mode, so we can verify the functionality is ok in our deployment. So I'd suggest fetch modules as usual but not actually build modules in MBS with dry_runn mode.

I agree with @qwan here.

There is no reason to bypass whitelist check in dry_run mode.

This is unnecessary.

I don't think we need to break into long if-else here, since we can handle that in mbs.py.

And we need fake event of module built to test module rebuild on the depend case.

And there is another PR #222 which also include the dry run for module build case, I don't mind to rebase it if you want improve this one on the dry run feature.

I would keep this here. I already put dryrun in mbs.py. They have different test purpose in my mind.

I guess you should mean to rebuild dependent modules of a built module in MBS, right? If yes, I don't think the fake event is required here. The normal logic does not do this. Dry run in this handler also does not aim to following that logic as well. Instead, if necessary, dry run should be added in handler MBSModuleStateChangeHandler.

That's not true, when a module built in MBS, no matter it's triggerred by our GitModuleMetadataChangeHandler nor MBSModuleStateChangeHandler, we're expecting to see a module state change message from MBS.

What code in this handler does it?

This is redundant when you need to handle the dry run in mbs.py, otherwise you'll need to repeat this in every module build handle, just like this PR, you'll need handle it in both git/module_metadata_change.py and mbs/module_state_change.py. And when there is any module build handler add in the future, we need to repeat it too.

MBS do it as we send module build request to MBS, but we don't do that in dry run mode, so we need inject the fake event by ourselves, otherwise we're unable to test the depend case with dry run mode.

Ok, I believe your comment is based on what you are thinking about how to test or verify the module rebuild. And my understand of your comments is you want to inject that fake event to have freshmaker start to rebuild all dependent modules in dry run mode. Am I right?

From my point of view, this is not redundant. Instead, during my test, it gives me clear messages to track what freshmaker does while handling a message. These lines of dry run code aims to follow the handler logic, those code in the build_module in mbs.py only follow the logic of what it does to request a module build from MBS. Different purposes here.

If you feel these code is not readable enough, we can think about how to improve the code structure.

Yes, that's why we need the dry run mode (to test our functionality without changing anything in real systems like koji and MBS), so the dry run mode should be able to simulate the real logic as possible as we can, isn't it? :)
And for the injecting stuff, it's required than better to have, because without that, we're unable to test our feature.

Yes, that's why we need the dry run mode (to test our functionality without changing anything in real systems like koji and MBS), so the dry run mode should be able to simulate the real logic as possible as we can, isn't it? :)
And for the injecting stuff, it's required than better to have, because without that, we're unable to test our feature.

Well, we are not on the same page totally. Even the dry run is for functional test here, I still don't think a fake event should be injected from dry run code to trigger dependent modules rebuild. Instead, for a functional test, freshmaker should wait for the MBS event and start rebuild in order to verify it works as expected.

My test method in my local machine is to run freshmaker and replay a MBS message to fedmsg-relay. These dry run code work well for me. And this should be flexible in a server, e.g. stg.

Injecting a fake event manually would be another choice for tracking what freshmaker does in server, but it depends.

Well, we are not on the same page totally. Even the dry run is for functional test here, I still don't think a fake event should be injected from dry run code to trigger dependent modules rebuild. Instead, for a functional test, freshmaker should wait for the MBS event and start rebuild in order to verify it works as expected.

The dry run mode is designed to verify the functionality in our deployment because we can't verify with real message from koji/mbs and we don't want to change anything in koji/mbs while testing.

My test method in my local machine is to run freshmaker and replay a MBS message to fedmsg-relay. These dry run code work well for me. And this should be flexible in a server, e.g. stg.
Injecting a fake event manually would be another choice for tracking what freshmaker does in server, but it depends.

That's is impossible in our deployment, and again that's why the dry run mode is added. @jkaluza, correct me if I'm wrong.

To be clear, I'm not saying to replay a message in a deployed server. Freshmaker is able to wait for messages from fedmsg hub.

To be clear, I'm not saying to replay a message in a deployed server. Freshmaker is able to wait for messages from fedmsg hub.

If we're expecting message from fedmsg hub, why we add dry run mode? why not just kick off the builds and then wait for the messages?

I would move this id handling to MBS class. It could just directly generate the right "id" right there in MBS class instead of counting it later in handlers.

Guys, this one seems to be stuck/stalled.

@cqi, you proposed it; would you like to pursue it or abandon it?

@ralph I'm going to pursue it. There are some conflicts and I'll fix them tomorrow.

Hey guys, I'm going to close this to get it out of the PR queue. @cqi, let's re-open it after the ursa work wraps up.