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

command/views: main View type aware if it's running in automation #28655

Merged
merged 2 commits into from
May 10, 2021

Conversation

apparentlymart
Copy link
Contributor

This "running in automation" idea is a best effort thing where we try to avoid printing out specific suggestions of commands to run in the main workflow when the user is running Terraform inside a wrapper script or other automation, because they probably don't want to bypass that automation.

This just makes that information available to the main views.View type, so we can then make use of it in the implementation of more specialized view types that embed views.View.

However, nothing is using it as of this commit. I intend to make use of it in #28634.

@apparentlymart apparentlymart requested a review from a team May 8, 2021 00:01
@apparentlymart apparentlymart self-assigned this May 8, 2021
@apparentlymart apparentlymart added the 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label May 8, 2021
Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable thing to add to the global view.

I'm not sure how you intend to use this in the follow-up PR. The plan, refresh, and apply views already have an inAutomation field which is set on init. Maybe you intend to add it to the show view as well?

If we do add it to the base view, can we then remove it from the plan/refresh/apply views and their constructors?

@apparentlymart
Copy link
Contributor Author

apparentlymart commented May 10, 2021

Ahh, I honestly hadn't noticed that this was already available on the higher-level view objects!

I guess the trick with what I was working on here is that "render plan" is an operation common across plan, apply, and show and so the pre-existing renderPlan function I was modifying in #28634 is therefore built in terms of the views.View type directly, rather than any of the command-specific views.

From a brief look at the constructors you referred to here it seems like it shouldn't be too big a refactor to change that about, so I'll give that a go shortly and push up a new version of this if it works out. Thanks for pointing that out!

This "running in automation" idea is a best effort thing where we try to
avoid printing out specific suggestions of commands to run in the main
workflow when the user is running Terraform inside a wrapper script or
other automation, because they probably don't want to bypass that
automation.

This just makes that information available to the main views.View type,
so we can then make use of it in the implementation of more specialized
view types that embed views.View.

However, nothing is using it as of this commit. We'll use it in later
commits.
We now have RunningInAutomation has a general concern in views.View, so
we no longer need to specify it for each command-specific constructor
separately.

For this initial change I focused only on changing the exported interface
of the views package and let the command-specific views go on having their
own unexported fields containing a copy of the flag because it made this
change less invasive and I wasn't feeling sure yet about whether we
ought to have code within command-specific views directly access the
internals of views.View. However, maybe we'll simplify this further in
a later commit if we conclude that these copies of the flag are
burdensome.

The general version of this gets set directly inside the main package,
which might at some future point allow us to make the command package
itself unaware of this "running in automation" idea and thus reinforce
that it's intended as a presentation-only thing rather than as a
behavioral thing, but we'll save more invasive refactoring for another
day.
@apparentlymart apparentlymart force-pushed the f-view-in-automation branch from 550aa28 to 330acba Compare May 10, 2021 17:21
@apparentlymart
Copy link
Contributor Author

@alisdair I've added another commit here responding to your comment about changing the command-specific view constructors. It seemed to work out pretty well but I stopped short of removing the individual fields for now because I wanted to keep this focused on the exported interface only for the moment. My commit message for the new commit contains some additional context:

command/views: Remove command-specific runningInAutomation

We now have RunningInAutomation has a general concern in views.View, so
we no longer need to specify it for each command-specific constructor
separately.

For this initial change I focused only on changing the exported interface
of the views package and let the command-specific views go on having their
own unexported fields containing a copy of the flag because it made this
change less invasive and I wasn't feeling sure yet about whether we
ought to have code within command-specific views directly access the
internals of views.View. However, maybe we'll simplify this further in
a later commit if we conclude that these copies of the flag are
burdensome.

The general version of this gets set directly inside the main package,
which might at some future point allow us to make the command package
itself unaware of this "running in automation" idea and thus reinforce
that it's intended as a presentation-only thing rather than as a
behavioral thing, but we'll save more invasive refactoring for another
day.

Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Good call limiting the refactor to the public interface! This looks great to me.

@apparentlymart apparentlymart removed the 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label May 10, 2021
@apparentlymart apparentlymart merged commit b402fd9 into main May 10, 2021
@apparentlymart apparentlymart deleted the f-view-in-automation branch May 10, 2021 17:50
@apparentlymart
Copy link
Contributor Author

Backported to v0.15 as 1db7b96 and 0a3bc35.

@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants