Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure logger is provided #10

Merged
merged 3 commits into from
Jan 7, 2022
Merged

Conversation

kgeckhart
Copy link

I started by fixing an issue where the logger wasn't being passed to the two AWS "interface" structs. I created some factory functions to use which should hopefully be more helpful vs the nil pointer crashes I got in dev.

I also did some renames for clarity on the AWS adapters and took a pass for structured logging so logs should have the same properties now.

Copy link

@matthewnolf matthewnolf left a comment

Choose a reason for hiding this comment

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

LGTM. This does mean we're drifting more from upstream repo. Do we have thoughts on what the longer term plan is for managing that?

Copy link

@ferruvich ferruvich left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Just wondering how many differences we're having from the forked repo, and the difficulty to eventually pull something useful from them.

@kgeckhart
Copy link
Author

I'm still planning to submit PR's for all of this upstream after I get things tested.

@kgeckhart
Copy link
Author

Thinking about it a bit more, I'm going to remove the renames from this PR since they are unrelated and are likely to cause more conflicts upstream.

@kgeckhart kgeckhart force-pushed the keckhart/ensure-logger-is-provided branch from 644cce3 to 9896146 Compare January 7, 2022 13:43
@kgeckhart kgeckhart force-pushed the keckhart/ensure-logger-is-provided branch from 9896146 to 86a04a5 Compare January 7, 2022 13:48
@kgeckhart kgeckhart merged commit 1bc09af into live Jan 7, 2022
@kgeckhart kgeckhart deleted the keckhart/ensure-logger-is-provided branch January 7, 2022 13:52
cristiangreco pushed a commit that referenced this pull request Apr 1, 2022
* Pass logger to the different structs

* Use a builder for the AWS services

* Use job specific loggers to propagate properties
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.

3 participants