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

Vue3 fix workload storage #12070

Conversation

mantis-toboggan-md
Copy link
Member

@mantis-toboggan-md mantis-toboggan-md commented Sep 30, 2024

Summary

Fixes #11957
Fixes #12020

Occurred changes and/or fixed issues

This PR includes a few fixes for pod and container storage.

Technical notes summary

#11957 - corrected issue with codemirror not rendering
#12020 - fixed issue with slot content not rendering + issues with prop mutation in ContainerMountPaths

Areas or cases that should be tested

  • view config of a workload with projected volume source
  • edit config of a workload with projected volume source
  • create/edit a workload with volumes and container volume mounts

You will need a deployment with a projected volume defined to test #11957

sample workload
apiVersion: apps/v1
kind: Deployment
metadata:
  name: test-multi-vol
  namespace: default
spec:
  progressDeadlineSeconds: 600
  replicas: 1
  revisionHistoryLimit: 10
  selector:
    matchLabels:
      workload.user.cattle.io/workloadselector: apps.deployment-default-test-multi-vol
  strategy:
    rollingUpdate:
      maxSurge: 25%
      maxUnavailable: 25%
    type: RollingUpdate
  template:
    metadata:
      creationTimestamp: null
      labels:
        workload.user.cattle.io/workloadselector: apps.deployment-default-test-multi-vol
      namespace: default
    spec:
      containers:
        - image: nginx
          imagePullPolicy: Always
          name: container-0
          resources: {}
          securityContext:
            allowPrivilegeEscalation: false
            privileged: false
            readOnlyRootFilesystem: false
            runAsNonRoot: false
          terminationMessagePath: /dev/termination-log
          terminationMessagePolicy: File
      dnsPolicy: ClusterFirst
      restartPolicy: Always
      schedulerName: default-scheduler
      securityContext: {}
      terminationGracePeriodSeconds: 30
      volumes:
        - name: projected-test
          projected:
            defaultMode: 420
            sources:
              - serviceAccountToken:
                  expirationSeconds: 9001
                  path: token
              - configMap:
                  items:
                    - key: key0
                      path: path0
                  name: confimap-name
        - name: projected-test-1
          projected:
            defaultMode: 420
            sources:
              - serviceAccountToken:
                  expirationSeconds: 9001
                  path: token
              - configMap:
                  items:
                    - key: key1
                      path: path1
                  name: confimap1-name

Screenshot/Video

Screen.Recording.2024-09-30.at.8.02.48.AM.mov

Screenshot 2024-09-30 at 8 02 37 AM

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

@@ -22,7 +22,9 @@ export default class LabeledSelectPo extends ComponentPo {
}

clickLabel(label: string) {
return this.getOptions().contains('li', label).click();
const labelRegex = new RegExp(`^${ label } $`, 'g');
Copy link
Member Author

Choose a reason for hiding this comment

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

This caused issues for me when running tests locally because it'd match any dropdown option containing the label provided, eg clickLabel('default') might select an option labeled cluster-fleet-default-c-9jtrc-35cc406cb29a

Copy link
Contributor

Choose a reason for hiding this comment

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

Was it only happening locally? if so, any idea why? I think I may have run into something similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've only seen it locally, I'm guessing when the test runs in ci there aren't other namespaces containing default available. There are probably other po methods that are using .contains without a regular expression and brittle in the same way though.

@@ -10,7 +10,7 @@ export default class TabbedPo extends ComponentPo {
}

clickTabWithSelector(selector: string) {
return this.self().get(`${ selector }`).click();
return this.self().find(`${ selector }`).click();
Copy link
Member Author

Choose a reason for hiding this comment

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

Switching from get to find ensures we click the right tab when the page has multiple tab groups with the same tabs

// },
// need to refresh codemirror when the tab is opened and hash change === tab change
watch: {
'$route.hash': {
Copy link
Member Author

@mantis-toboggan-md mantis-toboggan-md Oct 1, 2024

Choose a reason for hiding this comment

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

Tabs emit events when they're clicked, which is usually how codemirror refreshes are triggered in the dashboard. However; watching route.hash here saves us having to call refresh from 3 components up, where the tabs live in this case.

Copy link
Contributor

@codyrancher codyrancher left a comment

Choose a reason for hiding this comment

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

Verified basic functionality:
image

Thanks for adding all the additional testing automation.

}

addVolumeButton() : ButtonDropdownPo {
// return this.self().find('[data-testid="container-storage-add-button"]');
Copy link
Contributor

Choose a reason for hiding this comment

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

aside from this dead code, everything lgtm from e2e test perspective

@mantis-toboggan-md mantis-toboggan-md merged commit f28214f into rancher:master Oct 3, 2024
31 checks passed
nwmac pushed a commit to nwmac/dashboard that referenced this pull request Oct 24, 2024
* fix workload storage codemirror not rendering

* workload storage default component  yamleditor instead of codemirror

* test editing projected vols

* add container mount test

* fix lint

* refactor deployment tests to improve retry-ability

* add to workoad storage tests and improve retry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants