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
Fix removing and adding a remote relation #7672
Conversation
e937ff8
to
6cbebb8
Compare
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.
Looks good overall, just a couple of smallish things.
} | ||
return ops | ||
tokenOps := r.removeRemoteEntityOps(s.SourceModel(), s.Tag(), token) |
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.
IMO it would be neater and less error-prone (less unintentional sweeping under the rug) if removeRemoteEntityOps
expected a non-empty token, and we only called it here in the case where GetToken above returned a nil error.
I guess it becomes moot if we move the token into the remote application doc though? Is that coming soon?
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.
Yes, the next PR will address this so I didn't worry too much here.
state/remoteentities.go
Outdated
return nil, errors.Trace(err) | ||
} | ||
if err == nil { | ||
// Token already exists, so remove first. |
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.
If it already exists, can't you just return jujutxn.ErrNoOperations? it appears that you're removing and importing the exact same thing?
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 being cleaned up next PR, but I've added an improvement here too.
6cbebb8
to
0392c64
Compare
state/remoteentities.go
Outdated
return nil, jujutxn.ErrNoOperations | ||
} | ||
// Token already exists, so remove first. | ||
ops = append(ops, r.removeRemoteEntityOps(sourceModel, entity, token)...) |
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.
s/token/remoteEntity.Token/ ?
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.
doh!
0392c64
to
4a17b0b
Compare
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Build failed: Tests failed |
4a17b0b
to
d7b0f2f
Compare
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Description of change
If a cross model relation is removed and re-added, the relation would not be created again. This PR fixes that and makes other improvements:
The last 3 changes will be used in a subsequent PR to clean up remote entity references.
QA steps
Deploy a cmr scenario with mediawiki and mysql
remove relation -> media wiki is blocked
add relation again -> mediawiki is repaired