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

Respect PREFECT__CLOUD__SEND_FLOW_RUN_LOGS #6833

Merged
merged 12 commits into from Sep 19, 2022

Conversation

EmilRex
Copy link
Contributor

@EmilRex EmilRex commented Sep 14, 2022

The PREFECT__CLOUD__SEND_FLOW_RUN_LOGS environment variables is ignored when explicitly set. This change avoids overwriting it with a default value when set.

Closes #5521

@EmilRex EmilRex added the v1 Related to Prefect 1.x label Sep 14, 2022
@zanieb
Copy link
Contributor

zanieb commented Sep 14, 2022

Hm. Looking back at #4487 I do not think this setting is intended to be used this way. The engineer running the agent determines if flow run logs can be sent to Cloud. If I configure my agent not to send logs to Cloud, I would be pretty upset if any user could send logs by adding an environment variable.

Perhaps we should allow this to be turned off per flow run but never turned on unless already allowed at the agent level. We'll want to check that this meets Recursion's requirements.

As is, I think this could be seen as a security risk.

@EmilRex
Copy link
Contributor Author

EmilRex commented Sep 14, 2022

I would be pretty upset if any user could send logs by adding an environment variable.

Good point, I hadn't considered that.

Perhaps we should allow this to be turned off per flow run but never turned on unless already allowed at the agent level.

We'll want to check that this meets Recursion's requirements.
Agreed.

@johnurbanik
Copy link

Yes, I think that agent having flow logs off should take precedence, but if the agent has flow logs on, it should be possible to turn it off at the flow run level.

Updating the docs to communicate that clearly once implemented would also be great.

@zanieb
Copy link
Contributor

zanieb commented Sep 14, 2022

Yes, I think that agent having flow logs off should take precedence, but if the agent has flow logs on, it should be possible to turn it off at the flow run level.

Great! Definitely down for that implementation. @EmilRex you'll also need to implement this for all of the other agent types.

@EmilRex EmilRex requested a review from zanieb September 15, 2022 12:52
Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Can you write tests covering our new expected behavior as well?

@EmilRex EmilRex requested a review from zanieb September 19, 2022 16:44
@zanieb zanieb merged commit 63ced91 into 1.x Sep 19, 2022
@zanieb zanieb deleted the respect-cloud-send-flow-run-logs-vars branch September 19, 2022 19:00
@zanieb zanieb mentioned this pull request Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1 Related to Prefect 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants