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

fix(job_attachment)!: remove os_group field from Windows filesystem permission settings #215

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

gahyusuh
Copy link
Contributor

@gahyusuh gahyusuh commented Mar 15, 2024

What was the problem/requirement? (What/Why)

The os_group field in the WindowsFileSystemPermissionSettings was identified as unnecessary and marked for removal to simplify the interface.

What was the solution? (How)

To keep backward compatibility, we planned 3-phase removal:

  1. Make the file Optional (PR: fix(job_attachment)!: use username instead of group for Windows file permissions setting #196)
  2. Update the dependent package to stop passing os_group when creating WindowsFileSystemPermissionSettings object. (PR: chore(deps): upgrade to openjd-sessions 0.7.* and deadline 0.40.* deadline-cloud-worker-agent#213)
  3. Completely remove the field.

Steps 1 and 2 had been done. This PR is for Step 3.

Also, removed the PosixFileSystemPermissionSettings in the models.py. This whole class was moved to os_file_permissions.py, and we already fixed worker agent to import PosixFileSystemPermissionSettings from os_file_permission. (PR: aws-deadline/deadline-cloud-worker-agent#81)

What is the impact of this change?

We simplify the interface.

How was this change tested?

  • Unit tests: hatch run lint && hatch run test
  • Integ tests: hatch run integ:test
  • Ran a quick E2E test to make sure that removing PosixFileSystemPermissionSettings in the models.py does not break anything, and job submission & worker's input syncing are working as usual.

Was this change documented?

Is this a breaking change?

This is breaking change for any package that still uses the os_group field in the WindowsFileSystemPermissionSettings class. The only place is was being used was deadline-cloud-worker-agent, and we have already done the Step 2 to remove that usage.

@gahyusuh gahyusuh marked this pull request as ready for review March 15, 2024 14:29
@gahyusuh gahyusuh requested a review from a team as a code owner March 15, 2024 14:29
@ddneilson ddneilson changed the title chore(job_attachment)!: remove os_group field from Windows filesystem permission settings fix(job_attachment)!: remove os_group field from Windows filesystem permission settings Mar 15, 2024
Copy link
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

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

Thanks! I've updated the title from chore to fix -- that ensures that this breaking change shows up in the changelog at release. Chores don't get listed, and it's important that breaking changes get listed.

@gahyusuh gahyusuh force-pushed the gahyusuh/remove_os_group_windows branch from c570ea1 to 7435337 Compare March 15, 2024 16:32
@gahyusuh
Copy link
Contributor Author

Thank you @ddneilson for updating the title, I have fixed the commit message to match the title.

@gahyusuh gahyusuh merged commit 739cb20 into mainline Mar 15, 2024
18 checks passed
@gahyusuh gahyusuh deleted the gahyusuh/remove_os_group_windows branch March 15, 2024 16:42
npmacl pushed a commit that referenced this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants