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)!: Change osType and source_os names #37

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

gahyusuh
Copy link
Contributor

@gahyusuh gahyusuh commented Sep 19, 2023

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

The field osType: OperatingSystemFamily in job/attachments/manifests is currently being used just to determine how to handle the rootPath (i.e., is it 'posix' or 'windows',) in the API, so it follows more closely with the path mapping rules rather than storage profiles. The field osType in job attachment settings should be renamed to something else, to avoid inconsistencies around the usage of osType, osFamily, and whatever encapsulates 'windows' vs. 'posix' paths.

What was the solution? (How)

What is the impact of this change?

The field has a better/less-confusing name.

How was this change tested?

  • Ran unit tests: hatch run lint && hatch run test
  • Run an end-to-end test (submitting a non-rendering job bundle --> running a CMF against my local backend)

Was this change documented?

No.

Is this a breaking change?

Yes!

@gahyusuh gahyusuh force-pushed the gahyusuh/rename_os_type branch from aeb1efa to 7abe659 Compare September 19, 2023 18:36
@gahyusuh gahyusuh force-pushed the gahyusuh/rename_os_type branch 3 times, most recently from 03ad36f to e2e4fe4 Compare September 25, 2023 20:28
@gahyusuh gahyusuh marked this pull request as ready for review September 25, 2023 20:30
@gahyusuh gahyusuh requested a review from a team as a code owner September 25, 2023 20:30
# TODO: remove sourceOs and make sourcePathFormat required
sourceOs: NotRequired[str]
"""The operating system family (posix/windows) associated with the source path"""

sourcePathFormat: NotRequired[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

The above todo mentions that this should be made required, is that no longer true? Just want to make sure we didn't miss something

Copy link
Contributor Author

@gahyusuh gahyusuh Sep 25, 2023

Choose a reason for hiding this comment

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

Thank you for pointing that out. It appears, as you mentioned, this sourcePathFormat is defined as required in the backend service model. I will fix it in the next revision.

BREAKING CHANGES:
- Renamed the field `osType` -> `rootPathFormat`
- The workarounds that allowed the use of `source_os` in the path mapping rules are removed.

Signed-off-by: Gahyun Suh <[email protected]>
@gahyusuh gahyusuh force-pushed the gahyusuh/rename_os_type branch from e2e4fe4 to 503484c Compare September 25, 2023 21:03
@gahyusuh gahyusuh merged commit 5a39c25 into mainline Sep 25, 2023
8 checks passed
@gahyusuh gahyusuh deleted the gahyusuh/rename_os_type branch September 25, 2023 21:07
gmchale79 pushed a commit that referenced this pull request Oct 31, 2023
BREAKING CHANGES:
- Renamed the field `osType` -> `rootPathFormat`
- The workarounds that allowed the use of `source_os` in the path mapping rules are removed.

Signed-off-by: Gahyun Suh <[email protected]>
Signed-off-by: Graeme McHale <[email protected]>
gmchale79 pushed a commit that referenced this pull request Nov 2, 2023
BREAKING CHANGES:
- Renamed the field `osType` -> `rootPathFormat`
- The workarounds that allowed the use of `source_os` in the path mapping rules are removed.

Signed-off-by: Gahyun Suh <[email protected]>
Signed-off-by: Graeme McHale <[email protected]>
gahyusuh added a commit that referenced this pull request Nov 6, 2023
BREAKING CHANGES:
- Renamed the field `osType` -> `rootPathFormat`
- The workarounds that allowed the use of `source_os` in the path mapping rules are removed.

Signed-off-by: Gahyun Suh <[email protected]>
Signed-off-by: Graeme McHale <[email protected]>
Signed-off-by: Gahyun Suh <[email protected]>
gmchale79 pushed a commit that referenced this pull request Feb 12, 2024
BREAKING CHANGES:
- Renamed the field `osType` -> `rootPathFormat`
- The workarounds that allowed the use of `source_os` in the path mapping rules are removed.

Signed-off-by: Gahyun Suh <[email protected]>
Signed-off-by: Graeme McHale <[email protected]>
gmchale79 pushed a commit that referenced this pull request Mar 11, 2024
BREAKING CHANGES:
- Renamed the field `osType` -> `rootPathFormat`
- The workarounds that allowed the use of `source_os` in the path mapping rules are removed.

Signed-off-by: Gahyun Suh <[email protected]>
Signed-off-by: Graeme McHale <[email protected]>
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.

3 participants