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

Correctly update version_comment #466

Merged
merged 1 commit into from
Sep 2, 2021

Conversation

smaeda-ks
Copy link
Contributor

#465

This PR fixes two issues.

  1. version_comment attribute is ignored on service creation
  2. version_comment value is wrongly assigned from s.Version.Comment as opposed to s.ActiveVersion.Comment
    s.Version contains the last version object regardless of its state (draft/active), so we always should look at s.ActiveVersion

@smaeda-ks smaeda-ks requested a review from Integralist August 24, 2021 07:16
@smaeda-ks smaeda-ks self-assigned this Aug 24, 2021
@smaeda-ks smaeda-ks added the bug label Aug 24, 2021
@smaeda-ks smaeda-ks changed the title correctly update version_comment Correctly update version_comment Aug 24, 2021
@bengesoff
Copy link
Contributor

A future improvement might be worth looping through the versions to find the one matching "cloned_version" and using the comment off that. Using s.ActiveVersion.Comment fixes the case when "activate" is true, but there could be some disparity still when "activate" is false.

@smaeda-ks
Copy link
Contributor Author

smaeda-ks commented Sep 7, 2021

@bengesoff could you please elaborate on that more? I'm not following your point... sorry
In which scenario do we expect that to happen?


[updated]

I think I get your point. Thanks

@bengesoff
Copy link
Contributor

@smaeda-ks no problem - I think your fix is a definite improvement from where it was before, but I just figured it might be another edge case to catch 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants