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.

Sign up for GitHub

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

Jump to bottom

Better conflict handling for env var edits #1171

Merged
merged 1 commit into from
Jan 24, 2017
Merged

Better conflict handling for env var edits #1171

merged 1 commit into from
Jan 24, 2017

Conversation

spadgett
Copy link
Member

@spadgett spadgett commented Jan 23, 2017 •
edited
Loading

Fixes #621
Fixes #676

@jwforres @rhamilto FYI

Resources updated:

  • Deployment Config
  • Deployment
  • Replica Set
  • Replication Controller
  • Stateful Set

jwforres
jwforres reviewed Jan 23, 2017
View reviewed changes
app/scripts/controllers/deploymentConfig.js
previousEnvConflict = true;
$scope.alerts["background_update"] = {
type: "warning",
message: "This deployment configuration has been updated in the background. Saving your changes may create a conflict or cause loss of data.",
Copy link
Member

Choose a reason for hiding this comment

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

now that we know the env vars are what caused the conflict, i'm thinking we should update the message to be more specific

jwforres
jwforres requested changes Jan 23, 2017
View reviewed changes
app/scripts/services/environment.js
},

// Copy and normalize the environment for editing using the key value editor.
copy: function(object) {
Copy link
Member

Choose a reason for hiding this comment

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

hmm, i wouldn't expect a function called copy to also be doing normalization

app/scripts/services/environment.js

var i, leftEnv, rightEnv;
for (i = 0; i < leftContainers.length; i++) {
leftEnv = leftContainers[i].env || [];
Copy link
Member

Choose a reason for hiding this comment

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

i feel like we should be validating that the container names are actually the same, what if i took out a container, added a brand new container, the length of containers array is the same but they are no longer equal. Especially since merge edits below assumes containers in the same positions are in fact the same.

@spadgett spadgett force-pushed the merge-dc-env-vars branch 2 times, most recently from 9a3a06e to f74740c Compare January 23, 2017 20:30
@spadgett
Copy link
Member Author

@jwforres Updated. I'd like to do some more testing, but the code should be ready for another review.

@spadgett
Copy link
Member Author

Still need to sort out #676, which I suspect is a race between the PUT callback and watch callback.

jwforres
jwforres reviewed Jan 23, 2017
View reviewed changes
app/scripts/services/environment.js
return false;
}

// Check if any of the variable names are values are different.
Copy link
Member

Choose a reason for hiding this comment

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

typo in the comment 'variable names or values'

@jwforres
Copy link
Member

just one nit, otherwise LGTM

@spadgett
Copy link
Member Author

Thanks @jwforres

Updated with a fix for the PUT/watch race (#676).

spadgett
spadgett commented Jan 23, 2017
View reviewed changes
app/scripts/controllers/deployment.js
// Wait for a pending save to complete to avoid a race between the PUT and the watch callbacks.
if (saveEnvPromise) {
saveEnvPromise.finally(function() {
updateEnvironment(deployment, previous);
Copy link
Member Author

Choose a reason for hiding this comment

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

@jwforres It looks like calling updateEnvironment from saveEnvPromise.finally is enough.

  • If the PUT succeeds, the success callback in saveEnvVars will set the form to pristine before updateEnvironment is called. When the form is pristine, updateEnvironment will simply copy the new values.
  • If the PUT fails, the normal conflict checking is performed by updateEnvironment.

@jwforres
Copy link
Member

Update LGTM

@spadgett spadgett changed the title [WIP] Better conflict handling for env var edits Better conflict handling for env var edits Jan 23, 2017
@spadgett
Copy link
Member Author

Update LGTM

Thanks @jwforres. Tested various scenarios with all of the types (deployment configs, deployments, replication controllers, and replica sets). Also checked the read-only view of the stateful set environment.

[merge]

@spadgett spadgett mentioned this pull request Jan 23, 2017
Closed
@spadgett
Copy link
Member Author 

Running "add-redirect-uri" task
oc --server https://localhost:8443 patch oauthclient/openshift-web-console -p {"redirectURIs":["https://localhost:9000/"]}
>> Error: The connection to the server localhost:8443 was refused - did you specify the right host or port?

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to ac3b56f

@openshift-bot
Copy link

openshift-bot commented Jan 23, 2017 •
edited
Loading

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/974/) (Base Commit: e3d3db7)

@openshift-bot openshift-bot merged commit 3231986 into openshift:master Jan 24, 2017
@spadgett spadgett deleted the merge-dc-env-vars branch January 24, 2017 12:46
@spadgett spadgett mentioned this pull request Jan 24, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwforres jwforres jwforres requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@spadgett @jwforres @openshift-bot