-
Notifications
You must be signed in to change notification settings - Fork 49
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
Service key and project ID emulation settings tab for App Engine local run #2568
Conversation
@@ -38,3 +38,10 @@ devappserver.not.available=There is no App Engine development server available | |||
cannot.copy.appengine.api.sdk.jar=Cannot copy appengine-api-1.0-sdk.jar | |||
server.already.in.operation=Server is already in operation | |||
server.port=server port: {0,number,\#} | |||
|
|||
gcp.emulation.tab.name=GCP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cloud Platform
Codecov Report
@@ Coverage Diff @@
## master #2568 +/- ##
===========================================
+ Coverage 72.62% 72.93% +0.3%
- Complexity 2384 2436 +52
===========================================
Files 345 346 +1
Lines 13880 14107 +227
Branches 1387 1424 +37
===========================================
+ Hits 10081 10289 +208
- Misses 3230 3245 +15
- Partials 569 573 +4
Continue to review full report at Codecov.
|
|
||
@Test | ||
public void testUiComponents() { | ||
assertNotNull(accountSelector); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be at the end of setUp()
? At least, I don't see anything that causes the configuration to be different.
import org.eclipse.swt.widgets.Text; | ||
import org.eclipse.ui.PlatformUI; | ||
|
||
public class GcpEmulationTab extends AbstractLaunchConfigurationTab { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GcpEmulationTab
seems an odd name — I initially thought it was to do with unit testing. Would GcpProjectTab
be more representative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the name sounds a bit odd. I did thought about GcpProjectTab
, but that sounded too broad, while this tab only deals with providing the service key and the project ID for the convenience of users' local development. Any more suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GcpRuntimeCredentialsTab?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about GcpLocalRunTab
? It's not only for credentials; we have the project field.
public void initializeFrom(ILaunchConfiguration configuration) { | ||
accountEmailModel = getAttribute(configuration, ATTRIBUTE_ACCOUNT_EMAIL, ""); //$NON-NLS-1$ | ||
gcpProjectIdModel = getAttribute(configuration, ATTRIBUTE_GCP_PROJECT, ""); //$NON-NLS-1$ | ||
String serviceKey = getAttribute(configuration, ATTRIBUTE_SERVICE_KEY, ""); //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what happens if the user also adds a GOOGLE_APPLICATION_CREDENTIALS
or GOOGLE_CLOUD_PROJECT
value to the environment tab? Which wins?
One approach would be for this tab to actually store and retrieve its values from the launch config's environment data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a comprehensive plan, which I am already doing for Dataflow — it already has an account selector, whose login info is being passed as GOOGLE_APPLICATION_CREDENTIALS
. Basically, I will do the following:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you going to do for standard Java launches?
If you store values in the environment, then I think you're done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you going to do for standard Java launches?
I think we can just do the same thing. We anyway have to extend the standard Java run configs to add this tab. I just hope we can also extend the launch delegate so that we can extract values from our tab.
If you store values in the environment, then I think you're done.
That's true, but I'm afraid programmatically adding environment values there may confuse users and actually costs us more complexity in implementation. I think doing in the current manner is much simpler, and I don't see any issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you actually meant adding env vars to the launch config attribute, right? I thought you meant adding env vars to the Environments tab UI. I agree that is what we should do, now knowing that it seems not possible to extend the vanilla-Java launch delegate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One approach would be for this tab to actually store and retrieve its values from the launch config's environment data.
After some fiddling, I realized this doesn't work nicely, unless we tightly interwire the Environments tab and our tab. For example, suppose the current GOOGLE_APPLICATION_CREDENTIALS
is foo.json
. This value will be loaded in the service key field in our tab. The user will also be able to see it in the Environments tab. Now, in the Environments tab, suppose the user modifies this value to bar.json
. When the user presses Apply, what happens is that the value is immediately reverted back to foo.json
, because the framework will call performApply()
of our tab as well (even though our tab is invisible). This will overwrite bar.json
with foo.json
that is in the service key text field. This problem hit me, as I closed the run config right way not knowing that the value was reverted.
Basically, it seems not a good idea for a tab to make changes to a model of a different tab, unless you are willing to update UIs of the other tab simultaneously. I don't think we want to extend the Environments tab to hack this. It won't work in general cases either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to go a bit deeper and implement the activated()
and deactivated()
APIs. That way you'll know when to ignore your text field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that sounded like a bit of abusing the tab framework, but looks like we can utilize it to make things work reasonably acceptable. This results in some minute annoyances (confirming save even when values have restored to its original state, and another due to #2571), but I think they are pretty insignificant issues.
|
||
@Override | ||
public void createControl(Composite parent) { | ||
Composite composite = new Composite(parent, SWT.NONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this, and dataflow, should instead have a read-only representation with an Edit… button that pops this up as a dialog. That would avoid the dynamic background updates etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an attractive idea. I actually thought about having a read-only text field for the project (the account is just for the project selector), which can display the currently saved project even if the user is not logged in. Then we have a "Select Project..." button next to it, which pops up the project selector dialog. I really like this idea considering the benefits from the implementation perspective. The UI is fast because there won't be a long-running project fetch, no async issue in the run config, and a very clean model in the case of users not logged in. @elharo what do you think of this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elharo likes the idea too, so we can seriously consider going that route. We agreed that we first move forward with this PR (hoping to be able to release this soon) and implement the idea later. I don't think it will be more difficult to do it later than doing it now; it will be changes only in the UI.
I'm noticing that this PR seems to focus on the App Engine standard use-case. I've brought up concerns about that strategy in the design doc that I think should be resolved before we go far down this path. The focus of this feature was for library users in vanilla (non app-engine std environments) as a user looking at this UI I would ask myself a lot of questions that would require complex answers. That seems bad. e.g.
|
In fact, we are only focusing on the App Engine local run now. This doesn't mean we ignore non-App Engine cases. We just do this for App Engine initially. However, this tab component is designed to be universal — it can be reused in other run configs (such as plain Java run configs). I'll keep this universality in mind.
I've asked the same questions in the design doc. My comment is on the phrase "for selecting the project ID". We can continue our discussion there. I also push for adding something for clarity. |
For further clarification, there is nothing App Engine-specific in this PR. The tab is for setting a service key and a project ID, which can be applied to any run config. |
Let's not overthink this. If both the GCP and environment tabs set the credentials/project ID, pick one when launching the server. It's not worth extra effort to keep them in sync. |
@briandealwis the last commit changes this to save/load values to/from environment variables. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few thoughts below.
@Test | ||
public void testGetAttribute() throws CoreException { | ||
when(launchConfig.getAttribute(eq("attribute-key"), anyString())).thenReturn("expected value"); | ||
String value = GcpLocalRunTab.getAttribute(launchConfig, "attribute-key", "defualt"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling: default
} | ||
configuration.setAttribute(ILaunchManager.ATTR_ENVIRONMENT_VARIABLES, environmentMap); | ||
|
||
if (environmentTab != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, this is unfortunate :-(
Would it make sense to move this initializeFrom()
to deactivated()
? I suppose not, since EnvironmentTab.performApply()
may be called in between for other reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. But deactivated()
always calls performApply()
, so there is no difference anyway. Also, I think doing it here is on the safe side and fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think it's enough to move it into deactivated()
. performApply()
is called too frequently, and I can't think of any case where we have to update the environment tab before deactivation.
configuration.setAttribute(ILaunchManager.ATTR_ENVIRONMENT_VARIABLES, environmentMap); | ||
|
||
if (environmentTab != null) { | ||
// Needed to make the "Environment" tab to re-initialize its UI from the changes made here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add that EnvironmentTab
doesn't refresh the environment in its activated()
, so we … Ido you want to file a bug against Eclipse Platform/Debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear if it is a bug. Its comment says it won't do anything when activated/deactivated, which is OK from their perspective because everything just works when isolated. It's not working because we are modifying their model in a different tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's not their model: it's an ILaunchConfiguration
that is shared across the tabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateProjectSelector(); | ||
|
||
if (!initializingUiValues) { | ||
boolean emptySelection = accountSelector.getSelectedEmail().isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accountSelected
? empty selection doesn't describe the meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized somethingSelected
works better here.
boolean savedEmailAvailable = accountSelector.isEmailAvailable(accountEmailModel); | ||
if (!emptySelection || savedEmailAvailable) { | ||
accountEmailModel = accountSelector.getSelectedEmail(); | ||
gcpProjectIdModel = ""; //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it would be nice if the selected project was preserved if I change to account that can access the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fiddled with this issue for quite some time. That behavior would be desirable, but it is not possible to know ahead if the user will ever change back to the previous account to restore the preserved project, or the user really wanted to change the account and then just let the project not selected. (In the latter case, we should not provide a wrong project ID when actually running the server.) So, I think the current state of affair is what we should do, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I meant:
- I select account A1 and project P
- I then select A2 which has access to P
P should stay selected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case is not so different, and it is difficult to make it right, because it is assumed that the UI and the model are always in sync. Suppose the user changed to A1.5 between A1 and A2. If we could magically anticipate that the user will eventually change to A2 later, we could not clear P. However, if we do not clear P when selecting A1.5 and the user applies the change and exits, we will be wrongly providing P at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not suggesting retaining the last-selected project when switching A1 → A1.5 → A2. It makes sense that it's cleared.
That said, I'm not sure how often this will occur in practice. It doesn't happen with any of my current projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's leave this as-is. To make your scenario work, the account selector listener has to be coupled with the project selector (because it can only determine the situation after all the projects of A2 are loaded), which doesn't sound worth the effort.
In the end, all the complications and shortcomings will be eliminated by #2571.
projectSelector.setProjects(new ArrayList<GcpProject>()); | ||
} else { | ||
try { | ||
List<GcpProject> gcpProjects = projectRepository.getProjects(credential); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is potentially really slow. Perhaps it should be wrapped in a BusyIndicator
. See PipelineArgumentsTab#updatePipelineOptionsForm()
. (Or the callers should be wrapped.)
Label projectLabel = new Label(composite, SWT.LEAD); | ||
projectLabel.setText(Messages.getString("label.project")); //$NON-NLS-1$ | ||
|
||
Composite projectSelectorComposite = new Composite(composite, SWT.NONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it makes sense to extract this into a AccountProjectSelectionPanel
or something. We use this this combination in a couple of places, like the AppEngineDeployDialog
. And #1374 seems like it relevant here too — maybe we should turn ProjectRepository
into an injectable service that caches the credential-projects for some period of time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, we may decide to do so in the future. For now, the deploy dialog uses databinding, and things work quite differently there (e.g., auto-selecting a single account, it has a refresh button, it has the App Engine application checking and error messages, it has the project creation link, etc); the project selector there is very specialized. So, this would require a substantial work to factor out, so we'd probably want to consider it the next time we need the bare minimum combination similar to the one in this PR.
The cache idea is nice too.
setControl(composite); | ||
|
||
gcpIcon = SharedImages.GCP_IMAGE_DESCRIPTOR.createImage(); | ||
composite.addDisposeListener(new ImageDisposer(gcpIcon)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do this by overriding AbstractLaunchConfigurationTab.dispose()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out, the tab lifecycle doesn't seem to match the SWT lifecycle here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean — what is the effect? Because with this ImageDisposer
, gcpIcon
is still left pointing at an invalid object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some more testing. Looks like the tab's dispose()
is matched with class instantiation, which is different from that SWT's dispose()
is matched with createControl()
. I finally decided to make gcpIcon
instance-scoped; gcpIcon
is now a final
field, created only once when the class is created.
|
||
@VisibleForTesting | ||
static Map<String, String> getEnvironmentMap(ILaunchConfiguration configuration) { | ||
Map<String, String> emptyMap = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collections.emptyMap()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, we need a mutable map.
String serviceKey = Strings.nullToEmpty(environmentMap.get(SERVICE_KEY_ENVIRONMENT_VARIABLE)); | ||
|
||
initializingUiValues = true; | ||
accountSelector.selectAccount(accountEmailModel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth mentioning here that the account-selection-event listener updates the project list and waits until complete. Which isn't great for UI liveness, but …
I believe all the comments are addressed. PTAL. |
// To prevent updating above models when programmatically setting up UI components. | ||
private boolean initializingUiValues; | ||
|
||
private boolean activated; // to avoid https://github.com/GoogleCloudPlatform/google-cloud-eclipse/pull/2568#discussion_r150128582 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* true if this tab is the currently visible tab
* @see #performApply(ILaunchConfigurationWorkingCopy) for details
*/
`
updateProjectSelector(); | ||
|
||
if (!initializingUiValues) { | ||
boolean somethingSelected = !accountSelector.getSelectedEmail().isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accountSelected
?
boolean somethingSelected = !accountSelector.getSelectedEmail().isEmpty(); | ||
boolean savedEmailAvailable = accountSelector.isEmailAvailable(accountEmailModel); | ||
if (somethingSelected || savedEmailAvailable) { | ||
accountEmailModel = accountSelector.getSelectedEmail(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right: if no account is currently selected, but our saved-email (accountEmailModel
) is available, then we throw away our saved-email.
Ohhh! I see: savedEmailAvailable==true
means the selector has been populated, and so we now update the accountEmailModel
to keep it in sync with changes. A comment would help here, and below for the project selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. This covers the case where users explicitly de-selected any selection. In this case, we toss the saved value. (Account deselection is not implemented yet, but we do have an issue for this enhancement). I'll add a comment.
@Override | ||
public void selectionChanged(SelectionChangedEvent event) { | ||
if (!initializingUiValues) { | ||
boolean somethingSelected = !projectSelector.getSelectProjectId().isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
projectSelected
?
return; | ||
} | ||
|
||
BusyIndicator.showWhile(Display.getCurrent(), new Runnable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to use serviceKeyInput.getDisplay()
.
String serviceKey = Strings.nullToEmpty(environmentMap.get(SERVICE_KEY_ENVIRONMENT_VARIABLE)); | ||
|
||
initializingUiValues = true; | ||
// Selecting an account loads projects into the project selector synchronously (via a listener). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be wrapped in a BusyIndicator
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selecting an account itself should be instant. It is the project selector that makes network calls. So, I put BusyIndicator
around the project selector listener.
|
||
@Override | ||
public void performApply(ILaunchConfigurationWorkingCopy configuration) { | ||
if (!activated) { // to avoid https://github.com/GoogleCloudPlatform/google-cloud-eclipse/pull/2568#discussion_r150128582 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably best to include a potted summary here, in case these projects are moved elsewhere, or Github disappears.
must avoid updating the environment map unless we're active as performApply() is when the user makes changes in other tabs, like the EnvironmentTab, and thus we could clobber changes made in the EnvironmentTab
|
||
@VisibleForTesting | ||
static Map<String, String> getEnvironmentMap(ILaunchConfiguration configuration) { | ||
Map<String, String> emptyMap = new HashMap<>(); // should be mutable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is unexpected? An explanation would be useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted to signify we shouldn't use Collections.emptyMap()
which is immutable. I'll update the comment.
@@ -68,6 +67,8 @@ | |||
private static final String SERVICE_KEY_ENVIRONMENT_VARIABLE = | |||
"GOOGLE_APPLICATION_CREDENTIALS"; //$NON-NLS-1$ | |||
|
|||
private final Image gcpIcon = SharedImages.GCP_IMAGE_DESCRIPTOR.createImage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach isn't a good practice as the instance may be created but never used (and hence the image is created unnecessarily), and there's no guarantee that the instance will be created on the SWT thread. Better would be for getImage()
to create the image lazily and dispose()
to do a check-if-null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that makes sense. BTW, the image doesn't need to be created on the UI thread.
Only the UI, which is even not visible (not wired). No validation yet —what works is just selecting/retrieving/entering/saving values through UI. The service key path input is particularly incomplete; the browse... button does nothing.
A lot of part is straightforward. However, there is one trick to make things work reasonably sound in the case where an account and a project ID are saved in a run config but the account is not logged in; it was especially tricky to make it right in the run config dialog.
It's loading projects synchronously, but it's not so bad even with a lot of projects. If we do it async in the future, I think we should be very careful. Unlike the deploy panel, I can see many potential pitfalls when doing async in the run config dialog. It's easy to go into the current mess of the Dataflow run config.