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

Set log.Default's output to io.Discard #678

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Mar 19, 2024

Description of your changes

This PR sets a default io.Discard logger for the controller-runtime if debug logging is not enabled. If debug logging is enabled, then the controller-runtime uses a debug mode zap logger as usual.

It also sets the log.Default's output to io.Discard. According to a test with a ResourceGroup.azure, this prevents the noisy logs from the underlying Terraform provider. We still need to check for direct log messages via the fmt variants, such as fmt.Println.

In a further iteration, we will also consider making the underlying provider's logs available in a structured format.

This PR also fixes the dynamic reference resolution for the MRs in the azure.upbound.io group by passing the -o azure.azure.upbound.io=azure.upbound.io command-line option to the resolver transformer.

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Tested manually with the ResourceGroup.azure resource and the Subnet.network resource via upjet here.

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/azure/resourcegroup.yaml"

Copy link
Collaborator

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @ulucinar LGTM! I also validated the provider logs in dumps. We do not print the huge log about the provider configuration anymore. It seems that the size of the dumps are also affected via this change. Before, for the resourcegroup.yaml test, our dump's size is more than 2 MB. Now this is around 600 KB. This is another validation point about the logging improvements.

@turkenf
Copy link
Collaborator

turkenf commented Mar 21, 2024

/test-examples="examples/network/subnet.yaml"

if debug logging is not enabled.

- Set log.Default's output to io.Discard.

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/azure/resourcegroup.yaml"

…lver transformer

to fix reference resolutions for the MRs in the "azure.upbound.io" API group.

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/network/subnet.yaml"

@ulucinar
Copy link
Collaborator Author

make lint succeeded in a local run...

@ulucinar ulucinar merged commit 1570f5a into crossplane-contrib:main Mar 21, 2024
9 of 10 checks passed
@ulucinar ulucinar deleted the fix-logs branch March 21, 2024 12:49
Copy link

Backport failed for release-0.42, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-0.42
git worktree add -d .worktree/backport-678-to-release-0.42 origin/release-0.42
cd .worktree/backport-678-to-release-0.42
git checkout -b backport-678-to-release-0.42
ancref=$(git merge-base bd59c298108d23e906d9fcbba860507e12cd163e 6790d291dd56eb32c439b2f1b5e7313da6c5b9e3)
git cherry-pick -x $ancref..6790d291dd56eb32c439b2f1b5e7313da6c5b9e3

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

Successfully merging this pull request may close these issues.

3 participants