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

Rename osim #390

Merged
merged 4 commits into from
Dec 5, 2023
Merged

Rename osim #390

merged 4 commits into from
Dec 5, 2023

Conversation

costaconrado
Copy link
Contributor

This PR renames OSIM module to Workflows.

In order to achieve that this PR is separated in 4 commits that respectively has the purpose of:

  • Change the folder name modifying imports, settings and comments/documentations;
  • Change classes, exceptions and models names and attributes to comply with new naming;
  • Change constants names related to the module;
  • Change the API path from /osim/* to /workflows/*.

Closes OSIDB-1395

@costaconrado costaconrado force-pushed the rename-osim branch 2 times, most recently from 430878a to 6ae9376 Compare November 27, 2023 22:24
@costaconrado costaconrado requested a review from a team November 27, 2023 23:05
Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

LGTM

This is obviously a breaking change so to be really precise we should release it as a new major version. We could argue that it was an experimental module up until now - no sure if it would be safe anyway @JakubFrejlach FYI.

Or maybe it is already a good time to do a major release? Maybe at point when we have all the task management done? Or maybe also trackers? (those are probably done API-wise) Waiting for the real GA would take more time.

WDYT @RedHatProductSecurity/osidb-devs ?

apps/workflows/apps.py Outdated Show resolved Hide resolved
@costaconrado
Copy link
Contributor Author

@osoukup my opinion on that is should at least wait until all task management is done so we don't break it twice for task management, about trackers I think that we could wait until its done so we only do one major release in a short period of time, if OSIM team need task management before that I believe we could just deploy it to stage for development... Should we wait for more opinions on that before merging this branch or can I merge it and move this discussion to slack?

@osoukup
Copy link
Contributor

osoukup commented Nov 29, 2023

@osoukup my opinion on that is should at least wait until all task management is done so we don't break it twice for task management, about trackers I think that we could wait until its done so we only do one major release in a short period of time, if OSIM team need task management before that I believe we could just deploy it to stage for development... Should we wait for more opinions on that before merging this branch or can I merge it and move this discussion to slack?

@costaconrado this discussion does not have to block the merge. I agree that it would be better to deliver this all together and ideally together with tracker workflow support to have only one major release.

@Elkasitu
Copy link
Contributor

WDYT @RedHatProductSecurity/osidb-devs ?

Can't we just mark the /osim/ path as deprecated and have both /osim/ and /workflows/ paths?

@costaconrado costaconrado force-pushed the rename-osim branch 9 times, most recently from 8435160 to e418dbf Compare November 30, 2023 01:23
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename this file to something more readable? :)

Copy link
Contributor Author

@costaconrado costaconrado Nov 30, 2023

Choose a reason for hiding this comment

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

I sure can, also noticed that I missed the last migration naming I will add a new PR for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!
Regarding the last migration which is already in master - I am not sure whether it is safe to rename it. We discussed something similar in past with Ondrej and Adrian. @osoukup or @Elkasitu, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@costaconrado I think we can leave the 104 with old name and just fix 105 :).

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the name of an existing migration can make some mess which is theoretically possible to clean manually but not trivial or save. I would leave the existing ones and just try to be more careful with the new ones.

On one hand the migrations are auto-generated and theoretically people should not need to read them but sometimes it may be handy or at least good to get some understanding etc.

I prefer human readable migrations so eg. I try to process them by black but as a team we once decided to generally exclude them from this so it is up to every individual.

@costaconrado costaconrado force-pushed the rename-osim branch 2 times, most recently from a53f5f9 to 24bdbab Compare November 30, 2023 14:27
Copy link
Contributor

@Elkasitu Elkasitu left a comment

Choose a reason for hiding this comment

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

Reason for skipping the failing OpenAPI check:

Apparently oasdiff likes to do deprecation in a very specific way and seems to not be correctly documented or buggy, so we've decided to ignore its errors as the endpoints are correctly marked as deprecated from our POV, we have also confirmed that this shouldn't inherently make the check fail in subsequent PRs

@costaconrado costaconrado added this pull request to the merge queue Dec 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 4, 2023
@costaconrado costaconrado added this pull request to the merge queue Dec 5, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 5, 2023
Signed-off-by: Conrado Costa <[email protected]>
@costaconrado costaconrado force-pushed the rename-osim branch 2 times, most recently from 56cc534 to 857f13b Compare December 5, 2023 12:28
@costaconrado costaconrado added this pull request to the merge queue Dec 5, 2023
Merged via the queue into master with commit 12930e3 Dec 5, 2023
10 of 11 checks passed
@costaconrado costaconrado deleted the rename-osim branch December 5, 2023 12:48
@costaconrado costaconrado restored the rename-osim branch December 5, 2023 13:13
github-merge-queue bot pushed a commit that referenced this pull request Dec 5, 2023
This PR changes tests using workflows APIs.

During #390 some tests were not proper changed for the new pattern of
the API and this introduced faulty tests on master branch.
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