-
Notifications
You must be signed in to change notification settings - Fork 2
Standardize the sync/suspend/edit buttons #3487
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
Conversation
b8f2350 to
ea39bfa
Compare
dfe7f2d to
af8c6a2
Compare
43a8030 to
fca4de8
Compare
9d8f274 to
854dbb4
Compare
99f807a to
5863c1f
Compare
|
Put on hold until Thursday because of failing tests (breaking changes in OSS). |
5863c1f to
6f6de48
Compare
90f7b3b to
7fdaf5e
Compare
7fdaf5e to
63b38fd
Compare
63b38fd to
47cf6d7
Compare
#3487) * Add `SyncControls` instead of custom controls on `GitOpsSet`, `Terraform` object, and `Secret` detail views. * Replace the icon EditButton with a text-based EditButton with the new Settings icon. * Update snapshots. * Fix related tests.
c0912cb to
7094be6
Compare
| clusterName: (resource as GetTerraformObjectResponse)?.object | ||
| ?.clusterName, | ||
| }); | ||
| case 'GitOpsSet': |
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.
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 see, in this case maybe editing GitOpsSet objects is not supported on the backend? Because I only added the edit button on the frontend (if I got the ticket requirements right).
Maybe we need to move adding Edit button to GitOpsSets to a separate ticket? I thought the backend part is finished already.
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 backend error message is from core/server/primarykinds.go in OSS:
// Lookup ensures that a kind name is known, white-listed, and returns
// the full GVK for that kind
func (pk *PrimaryKinds) Lookup(kind string) (*schema.GroupVersionKind, error) {
gvk, ok := pk.kinds[kind]
if !ok {
return nil, fmt.Errorf("looking up objects of kind %v not supported", kind)
}
return &gvk, nil
}
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 frontend error message visible on the screenshot is from a frontend check for template name in annotations:
const templateName = getCreateRequestAnnotation(resource)?.template_name;
if (!templateName) {
...
[show notification with text]
No edit information is available for this resource.
...
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 am not sure how a GitOpsSet is created from a template and if the template name should be added automatically or manually. Maybe this functionality should be added too.
| > | ||
| {object?.suspended ? 'Resume' : 'Suspend'} | ||
| </Button> | ||
| <SyncControls |
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.
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 dont see sync controls in terraform within the details page
Weird, they should be there. They are rendered non-conditionally.
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.
Have you pulled in the latest commit? Because I think I removed Sync controls on the TF object details page temporarily on rebase and re-added them later.
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.
In the terraform table page i could see controls but sync hangs
TF object sync always hangs for me even locally in the main branch, I have not changed any sync logic.
There was an old issue for it with sync timing out:
#2264
but it looks like it was fixed.
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.
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.
checking again, i was able to see the new controls in the other but terrraform UIs
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.
@enekofb weird, must be a caching issue (Tilt, yarn or browser), because I see them in the app, they are replaced in the code on TerraformObjectDetail view, and TerraformObjectDetail view tests are updated with the test IDs from the new controls.
Could you delete the node_modules folder and run make node_modules? And possibly restart everything (native-ui-build, compile, clusters-service, whatever is relevant) in Tilt manually and re-test?
I think I had a similar issue with UI not updating in the past once or twice.
@jpellizzari @joshri could any of you please check that the new Sync/Suspend controls are present on the Terraform object details view?
053334f to
1e4b621
Compare
#3487) * Add `SyncControls` instead of custom controls on `GitOpsSet`, `Terraform` object, and `Secret` detail views. * Replace the icon EditButton with a text-based EditButton with the new Settings icon. * Update snapshots. * Fix related tests.
1e4b621 to
a08069b
Compare
|
@enekofb @jpellizzari @joshri as discussed with @LappleApple , I removed the Edit GitOpsSet functionality (it might be added in another issue later), so the current PR can be reviewed. |
|
Integration tests are failing twice, restarting them. |
jpellizzari
left a comment
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.
LGTM 👍


Closes #3374
SyncControlscomponent.Notes:
Standardize the sync/suspend/edit buttons weave-gitops#4080