samvera-labs / active_encode

Declare encode job classes that can be run by a variety of encoding services
Other
6 stars 8 forks source link

MediaConvert adapter setup! improvements #90

Closed jrochkind closed 2 years ago

jrochkind commented 2 years ago

Improvements to the setup! method that is meant to set up a cloudwatch rule and log group suitable for the adapter to fetch output information from on complete.

One change was necessary for setup! to create a working setup:

Two changes that are improvements:

There are no existing automated tests for setup!, and I couldn't see a reasonable way to create any at this point. But I did test this manually, and the adapter is now/still succesfully finding the log event for output info on completion.

jrochkind commented 2 years ago

I think rubocopis wrong about one of those things:

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter. cloudwatch_logs.put_retention_policy({

In ruby 3.0 world, passing in a hash is NOT always the same thing as passing in keywords. If the thing wants one hash arg, that's differnet than wanting keyword args.

It's possible it will work without the braces, but the AWS SDK doc example use braces, so I'm more comfortable using them.

Can we disable that rubocop? (I'm kind of surprised that it's there at all, in a post ruby 2.7/3 world I would expect it to not be in rubocop anymore; but maybe the rubocop rules just haven't been updated in a while).

@cjcolvar , thoughts?

jrochkind commented 2 years ago

Looked at source in the AWS-sdk, the relevant method defintion is put_retention_policy(params = {}, options = {})

I think in ruby 2.7+, it is incorrect to pass them in as keyword options instead of a hash, and what rubocop is telling me to do is actually wrong.

mbklein commented 2 years ago

I'm OK with disabling that cop for this file if not for the whole repo. Either way.

jrochkind commented 2 years ago

Looks like it has indeed been removed from rubocop entirely, for exactly this reason:

https://review.discourse.org/t/this-rule-was-removed-from-rubocop-due-to-different-behavior-in-ruby-3/9230

Is CI somehow not using an up-to-date rubocop? Yup, we're stuck at rubocop <= 0.52.1 (this is December 2017 rubocop!), becuase that's required by bixby 1.0, which we are using. So using a really old set of rubocop rules is probably challenging.

If we updated to latest bixby 3.0.2, we'd be using rubocop 0.85.1. That's still June 2020, 18 months ago. (Rubocop current release is 0.25.1). I find it a bit annoying that samvera policies/conventions require us to use 18 month old rubocop?

I don't know how to disable rubocop rules in the repo. Unless someone else is rearing to, I can try to figure out how -- it should probably be in a different PR though?

Even once we disable that rule, it's still complaining. I do this (copy and paste of example from AWS SDK docs):

          cloudwatch_logs.put_retention_policy({
            log_group_name: name,
            retention_in_days: SETUP_LOG_GROUP_RETENTION_DAYS
          })

It complains:

Seriously? This is a totally normal way to do a method invocation with a hash literal. I can't figure out what to do that meets those rule that isn't a mess. Suggestions welcome for how to handle this elderly rubocop.

mbklein commented 2 years ago

I'll see if I can make it happy.

jrochkind commented 2 years ago

thank you @mbklein !