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

Small corrections to repo syncing and the behaviour of some gridview rows #3252

Merged
merged 3 commits into from
Nov 13, 2023

Conversation

kc284
Copy link
Contributor

@kc284 kc284 commented Nov 2, 2023

Please review each commit separately.

- Removed call of virtual member Enabled in constructor.
- Removed override of Enabled, because the method the setter called does
  nothing related to the row's enabled state.
- Minor syntax updates.

Signed-off-by: Konstantina Chremmou <[email protected]>
…ncWithCdnAction).

Signed-off-by: Konstantina Chremmou <[email protected]>
@kc284 kc284 self-assigned this Nov 2, 2023
@kc284 kc284 added the ITE PR should be reviewed within this iteration label Nov 2, 2023
@CitrixChris CitrixChris added the 1 approval PR has been approved by one reviewer label Nov 6, 2023
Copy link
Member

@danilo-delbusso danilo-delbusso left a comment

Choose a reason for hiding this comment

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

I'm approving because in general the PR can go in like this, my only comment is more of a nit-picky question so feel free to ignore

Comment on lines +125 to +131
var syncAction = new SyncWithCdnAction(pool);
syncAction.Completed += a =>
{
if (a.Succeeded)
CheckForCdnUpdates(a.Connection);
};
return syncAction;
Copy link
Member

Choose a reason for hiding this comment

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

Is there an instance whereby we sync and not check for updates? Since we always run CheckForCdnUpdates after SyncWithCdnAction, should we just be adding CheckForCdnUpdatesAction in SyncWithCdnAction:Run, or is that a side effect?

Usually I'd be against having one action do two things, but I don't see why we'd run sync without the check afterwards, so it sort of is the same action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason is that SyncWithCdnAction has to call CheckForCdnUpdates, not CheckForCdnUpdatesAction, and the former uses concepts (the Registry) which belong to XenAdmin, not XenModel, and it is not straight forward, if feasible, to move it because it blurrs the interface between business and UI logic (which is already not very clean in some areas).

@danilo-delbusso danilo-delbusso added 2 approvals PR has been approved by two reviewers and removed 1 approval PR has been approved by one reviewer labels Nov 13, 2023
@kc284 kc284 merged commit 055d9fa into xenserver:feature/cdn-updates Nov 13, 2023
1 check passed
@kc284 kc284 deleted the feature/cdn-updates branch November 13, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 approvals PR has been approved by two reviewers ITE PR should be reviewed within this iteration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants