Could you make the directory configurable?

You'd want to set this to all by default:
https://vcrpy.readthedocs.io/en/latest/usage.html#all

The reason is we want to record the HTTP requests, but we never want to replay the messages when Freshmaker is running in production.

Perhaps you can make this configurable so that when we want to debug an issue locally, we can change this value to none so that all interactions will come from the recorded cassettes:
https://vcrpy.readthedocs.io/en/latest/usage.html#none

The cassette should be named after the Freshmaker event. This allows someone who needs to debug Freshmaker locally to copy the single cassette for the specific Freshmaker event and replay it locally.

You should consider creating a pytest fixture that is automatically used which mocks vcrpy so that nothing is recorded while executing the unit tests.

The cassette should be named after the Freshmaker event. This allows someone who needs to debug Freshmaker locally to copy the single cassette for the specific Freshmaker event and replay it locally.

About this comment: do you mean that the file name you include the freshmaker event id? Mariana and I checked together and that information (the event id) is not available in that function. And it's not available also in the LightBlue base class. So we should change every method that calls the lightblue functions or that instantiate the LightBlue class to have the event info and than pass it to the _make_request method. It seems a bit too much of an effort for that info. Or do you have a smarter and easier idea in mind to get this info in there?

@gnaponie , @mprahl , can you take a look, please

Can you put a more detailed description here? Also for the mode.
Maybe mention that this is to record the lightblue requests.

Can you add a comment here explaining why we are doing this and what is this about? vcrpy is not that "famous" and some other developer might be confused by this. :)

Just to be more sure, could we check if self.event_id is defined?
you could do something like:
vcr_filename = f'event_id-{(str(self.event_id) if self.event_id else "")}-{str(int(time.time()))}.yml'
and then just:
with my_vcr.use_cassette(vcr_filename):

In my opinion, there should be no default value so that this functionality is disabled by default.

Could you please remove this extra new line?

Also the one below as well?

and save it => and save it if vcrpy is configured

I would move the vcrpy import to under this if statement so that it's only imported if it's enabled in the configuration. This allows someone that deploys Freshmaker to not have to have vcrpy installed if they don't use that functionality.

Don't use the timestamp in the filename here since we want all Lightblue requests to be recorded in a single cassette.

I'll leave this up to @gnaponie but I'd prefer if this functionality was documented as part of this PR.

In my opinion, there should be no default value so that this functionality is disabled by default.

That's a good point. @mulaieva let's do that.

Don't use the timestamp in the filename here since we want all Lightblue requests to be recorded in a single cassette.

I thought it would simply override the existing file... In that case, yeah, we can remove the timestamp. I'm still worried the self.event_id might be None (it would be a bug, but it could happen, also in case of new handlers that use Lightblue), so as a fall back maybe we could use the timestamp (only). Wdyt?

I'll leave this up to @gnaponie but I'd prefer if this functionality was documented as part of this PR.

Where would you document that? Maybe in https://docs.pagure.org/freshmaker/configuration.html ?

I'll leave this up to @gnaponie but I'd prefer if this functionality was documented as part of this PR.

Where would you document that? Maybe in https://docs.pagure.org/freshmaker/configuration.html ?

Yes, that seems like a reasonable place.

Don't use the timestamp in the filename here since we want all Lightblue requests to be recorded in a single cassette.

I thought it would simply override the existing file... In that case, yeah, we can remove the timestamp. I'm still worried the self.event_id might be None (it would be a bug, but it could happen, also in case of new handlers that use Lightblue), so as a fall back maybe we could use the timestamp (only). Wdyt?

I think event_id should be a mandatory parameter in the __init__ method or vcrpy should be disabled when it's not provided. If you go with the latter, an error or warning message should be logged.

Since in this page we are describing the configuration variables, you should put vcrpy_path and vcrpy_mode here and write something like configuring these 2 variables you enable the feature to record Lightblue queries per Freshmaker event. Or something similar to what you wrote, but pointing out the two variables names instead.

There's a typo: lightbule :)

We should put empty value here, in order to disable the feature by default.

for the recording => for recording

Import and record are performed if configuration variables => The importing of vcrpy and the recording of Lightblue queries is performed if the vcrpy configuration variables

It'd be nice if you had a _setifok_vcrpy_path method that tried to import vcrpy if this value is truthy. This would cause Freshmaker to fail when the container is started up rather than the first Lightblue query if vcrpy is enabled but not installed.

Optional:
It'd be nice to use an f-string:

with my_vcr.use_cassette(f'{self.event_id}.yml'):

Optional:
If you put these keyword argument outside of the if branch, you can just perform:

response = requests.post(entity_url, **request_kwargs)

This will allow you to not have to repeat your self below.

@mprahl , PTAL. Did I understand your notes right?

variables( => variables (

Ah, this must be in config.py. See how it's used:
https://pagure.io/fork/mulaieva/freshmaker/blob/11845f99d964298ba7c4b5c330f3e2114e5ab267/f/freshmaker/config.py#_432

This is what I had in mind for the method body:

def _setifok_vcrpy_path(self, s):
    if s:
        # This will raise an exception if vcrpy is not present and cause Freshmaker to fail to start
        import vcrpy
    self._vcrpy_path = s

You could also verify that s is a string.

Optional: It'd be easier to read if it was formatted like this:

        request_kwargs = {
            "data": json.dumps(data),
            "verify": self.verify_ssl,
            "cert": (self.cert, self.private_key),
            "headers": {'Content-Type': 'application/json'}
        }

I'll leave it up to @gnaponie to enforce this though since I have no authority over style in this repo :smile:

Please remove this after addressing my comments above

Remember, we want vcrpy to be disabled if vcr_path is not set.

if self.event_id and conf.vcrpy_path:

@mprahl, I've made a change in the tox.ini because I had a flake8 error here "./freshmaker/config.py:532:13: F401 'vcr' imported but unused".

Instead of disabling the check globally, just add # noqa: F401 at the end of this line.

After addressing my one remaining comment, this looks good to me! Nice job!

+1

:thumbsup:

Commit df5d2942 fixes this pull-request