-
Notifications
You must be signed in to change notification settings - Fork 45
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: deploy to a closed channel #661
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also need to document this behavior: see #647 (comment)
// lead to an inconsistent terraform state. This check is to error out to the user, | ||
// so they can purposely select the track. | ||
if deployInfo.Channel != transformedInput.charmChannel { | ||
return fmt.Errorf("you are trying to publish to a closed channel. Check the channel for the %s", transformedInput.applicationName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case the terraform plan is specifying a closed channel, so trying to deploy from a channel, not publish
// Ensure the new revision or channel is contained | ||
// in the origin to be saved by juju when AddCharm | ||
// is called. | ||
if input.Revision != nil { | ||
oldOrigin.Revision = input.Revision | ||
} else if input.Channel != "" { | ||
if newOrigin.Risk != resolvedOrigin.Risk { | ||
msg := "you've requested a risk different from the one resolved. Probably the channel was close" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not too fond of this message - it needs rewording
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yy, just want to show the approach before polishing the pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you wish to try out different solutions in a PR, make it a draft.
I would update the application resource documentation to make folks aware of the error and what the problem is instead.
if deployInfo.Channel != transformedInput.charmChannel { | ||
return fmt.Errorf("you are trying to publish to a closed channel. Check the channel for the %s", transformedInput.applicationName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're failing after the application has been deployed, but no local OCI image resources are uploaded? Thus the terraform state will not be saved, but the application exists in juju.
This sounds like a worse state to be in than the one you're trying to solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
Just a proposal, if accepted I will change the commit message, add tests and qa steps
Fix #647
To fix the issue we can return an error by checking the planned channel with the one received by the client. In this case if they are trying to publish to a closed channel they get a specific error and users can update their terraform plan to avoid incosistency.