-
Notifications
You must be signed in to change notification settings - Fork 84
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
1497819 - Broker should not rely on image field of APB yaml #433
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.
Just some minor things but I think are worth discussion.
pkg/broker/broker.go
Outdated
imageName := strings.Replace(spec.Image, ":", "-", -1) | ||
spec.FQName = strings.Replace(fmt.Sprintf("%v-%v", registryName, imageName), | ||
spec.FQName = strings.Replace( | ||
fmt.Sprintf("%v-%v", registryName, spec.FQName), |
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 worry that the FQName could have .
in it. If it does, this has caused issues with the service catalog in the past and might be worth replacing here.
pkg/apb/types.go
Outdated
@@ -62,7 +62,7 @@ type Plan struct { | |||
type Spec struct { | |||
ID string `json:"id"` | |||
FQName string `json:"name" yaml:"name"` | |||
Image string `json:"image"` | |||
Image string `json:"image,omitempty"` |
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 don't understand the need for this change. I don't think it will hurt but don't know why we need it.
I could see you doing yaml:"-"
so that we never take anything for the APB spec.
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 are correct. I don't believe I fully understand yet what the rules and side-effects are for struct tags. I couldn't find anything that detailed yaml:"-"
or the json/yaml struct tags in general, if there is anything you could point me to that would be helpful.
fc5a34e
to
a12fd9d
Compare
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.
ACK
Since this is a feature, do we want to wait on merging this until we branch? |
@djzager commit should be "bug # - commit message". Renamed the PR. |
The purpose of this change is to make it so that the full path to each image is used when executing APBs instead of relying on what is stated in the APB yaml. - Update the Spec to make Image optional (for when we remove it from APB yaml) - Update the registry adapters to store the full path to the image when unmarshalling the APB yaml -> APB spec before we store it into `etcd` - Update the broker to have simpler format for `FQName`
Update apb struct to not pull image from apb yaml. Also, update broker to use regex replace.
I believe this is ready now. |
…shift#433) * Bug 1497819 - Broker should not rely on image field of APB yaml The purpose of this change is to make it so that the full path to each image is used when executing APBs instead of relying on what is stated in the APB yaml. - Update the Spec to make Image optional (for when we remove it from APB yaml) - Update the registry adapters to store the full path to the image when unmarshalling the APB yaml -> APB spec before we store it into `etcd` - Update the broker to have simpler format for `FQName` * Do not read image from apb.yaml Update apb struct to not pull image from apb yaml. Also, update broker to use regex replace. * Change serviceclass names to match new FQName format
The purpose of this change is to make it so that the full path to each
image is used when executing APBs instead of relying on what is stated
in the APB yaml.
yaml)
unmarshalling the APB yaml -> APB spec before we store it into
etcd
FQName
Implements #431
Fixes #288
Bug 1497819