Skip to content

Extract log helper methods into a Logger::Formatter, Push the job_id into the Job object#6808

Merged
brrygrdn merged 6 commits intomainfrom
brrygrdn/extract-log-methods-from-updater
Mar 9, 2023
Merged

Extract log helper methods into a Logger::Formatter, Push the job_id into the Job object#6808
brrygrdn merged 6 commits intomainfrom
brrygrdn/extract-log-methods-from-updater

Conversation

@brrygrdn
Copy link
Copy Markdown
Contributor

@brrygrdn brrygrdn commented Mar 8, 2023

This branch extracts a couple of tidy ups from some ongoing work to prepare the Dependabot::Updater to receive our prototype grouped updating strategy.

While splitting the updater up into discrete classes, there were two cross-cutting pain points:

  • Having pass both a Job object and the job_id is counter productive and it leaves us reaching back to the Dependabot::Environment object as we start to extract code.
    • This is likely going to make compartmentalisation worse as the Environment object should never be part of Core but some of the Updater strategies we are extracting should become Core-bound eventually so ecosystems can override them
    • Solution: Let's just allow a Job to know its id?
  • Writing to the Dependabot.logger is wrapped in logger_info and logger_error methods in two levels of abstraction, the "Command" class and the Updater itself to inject a job_id prefix
    • Breaking the code out means they need access to a global logger with the prefix behaviour
    • We also have some limitations in our current logging layout that means we don't use debug level message which would really help both contributors and users a lot
    • Solution: Let's use Logger::Formatters to attach a presenter to the global logger we setup at the start of a job
    • Bonus points: Let's teach Dependabot::Environment about a debug flag so we can default to :info, but optionally invoke :debug

Service Changes

  • This branch introduces the use of a DEPENDABOT_DEBUG envvar, when 'true' it will set the log level to :debug.
  • The job.json, aka Job Definition File, can now also pass a job.debug = true value to set the log level on a job-by-job basis
  • The formatter is aware of github.com/dependabot/cli's placeholder Job ID and omits the prefix in that environment

@brrygrdn brrygrdn requested a review from a team as a code owner March 8, 2023 21:40
@brrygrdn brrygrdn force-pushed the brrygrdn/extract-log-methods-from-updater branch from fbede8b to a60d501 Compare March 8, 2023 21:52
Copy link
Copy Markdown
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the idea of a global level logger.

This all makes sense so far. I think you'll also need to define a way for the debug flag to be set on a job, but that's probably an API change?

I find it kind of weird that the CLI uses a job ID of cli instead of another flag to indicate its environment, but I guess that's a separate issue.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!!nil returns false here but this is kind of a weird way to do it?

Copy link
Copy Markdown
Contributor Author

@brrygrdn brrygrdn Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the "double-bang operator" is an idiom in Ruby to force any type into Boolean but I do always feel odd about using it over the present? helper - but that's a Rails-isim we don't have in Core as we don't want to include ActiveSupport anymore.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is be_debug_enabled defined?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is an RSpec-isim, be_debug_enabled is translated to environment.debug_enabled? to be true under the hood.

@brrygrdn
Copy link
Copy Markdown
Contributor Author

brrygrdn commented Mar 9, 2023

I find it kind of weird that the CLI uses a job ID of cli instead of another flag to indicate its environment, but I guess that's a separate issue.

Yeah, the context on that is that the Updater code path has a hard expectation that job_id is non-nil in a few different places. I think we can loosen that and maybe remove the need for the placeholder eventually. This PR helps with that a little but removing the need for job_id as a positional argument in any method which accepts the Job object, so it's baby steps towards maybe not needing the placeholder.

@brrygrdn brrygrdn force-pushed the brrygrdn/extract-log-methods-from-updater branch from 17bd8f6 to 43652c7 Compare March 9, 2023 11:26
@brrygrdn
Copy link
Copy Markdown
Contributor Author

brrygrdn commented Mar 9, 2023

This all makes sense so far. I think you'll also need to define a way for the debug flag to be set on a job, but that's probably an API change?

Yeah, this should be everything we need in Core, we can add debug flag setters to the various entry points like the dry-run.rb script. I've punted on that for now as we haven't started adding any .debug calls so we can probably just start using it as a dev tool and see how we get on for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants