squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-999488606
> > Thanks a lot for the review @astefanutti , it's a long one, I really appreciate your help with this. > > > My understanding is that the implementation assumes the "local file" use case is different than the "standard" ConfigMap use case. It's possible you've already tried to explain the difference, though could you please clarify the rational behind that assumption. It seems that the file prefix / reference gets stored in the container trait configuration, and even if it's weakly linked to the local file, and resolved to the generated ConfigMap, that seems to break the "standalone" configuration principle > > > > > > The idea we discussed was to go in the direction to [remove any arbitrary file content from the integration](https://github.com/apache/camel-k/issues/2320#issuecomment-922718785). With this PR what we do is to automatically help the user convert a local file into a configmap, and then attach to the Integration. In my opinion what we're doing is to remove the support for local file, although we help the user to convert it in the supported storage via `kamel` CLI to simplify his life. If the user is not able to run an Integration via `kamel run`, in fact, the feature cannot be provided. > > That is my background too. My question was more, why does the `file` prefix beyond the CLI, and the generated ConfigMap seems to be treated differently than the other "standard" ones? If I understood correctly, the question is about generated Configmap difference with user provided Configmap. The only difference is about the lifecycle of the Configmap itself, as my idea is to have it living together and exclusively within the Integration lifecycle. This is mimicking the previous behavior, when a Configmap was generated beside the Integration. The difference is that the resource is not directly stored in the Integration and the Configmap generated by the CLI (as it must access the file content). Another possibility would be to autogenerate the Configmap but let the lifecycle (update/delete) up to the user. However, I think it completely overlaps what we already provide with the normal configmap, just saving the manual step to create the configmap from the file resource manually. Hope it answer your question, I'm not entirely sure we're talking about the same thing here :) > > > > The generated ConfigMap is owned by the Integration, which makes the former gets garbage collected when the former is deleted. However, how is it garbage collected when the Integration is updated with the file resource removed, for example with a new kamel run invocation, without the resource? > > > > > > Good point, I did not consider this situation. The configmap is updated when there is a file change and `--sync/--dev` enabled, but I did not check what happens when the file is removed at all. I'll run some test and come back to this point. I made some test and with this implementation, the generated Configmap is kept until the Integration is living. We can provide an additional loop to reconcile the generated Configmap not needed right after the binding step. Or we can keep it the way it is, if we consider the patter to create an Integration with a file and later update the same integration without a file as a rare one. I don't have a strong opinion on this part. > > > What's the rational, from the functional standpoint, to use the container trait to host the configuration resources? It's already responsible for the compute resources, the container image, the service, and the (deprecated) health probes configuration. > > > > > > The container trait is already in charge to convert the configmaps/secret/... into mounted volumes ([we discussed it shortly as well](https://github.com/apache/camel-k/pull/2635#issuecomment-922720383)), so it felt the most appropriate trait to use. Otherwise we need somehow to move that logic elsewhere or duplicating it, which I don't think is a good idea. > > I'm trying to reason first from the end-user standpoint. The CLI hides it, still it's part of the API. I remember we had the discussion in #2635, and I was wondering whether that was the right moment to find a more "functionally oriented" trait to capture this concern, and disentangle the _container_ trait. The idea to move the _volumes_ logic into a separate trait could be interesting. However, we need to attach those volumes into the `corev1.Container`. If we create a new trait, then it will be very dependent on the container trait, requiring to be executed after it, and it will need to get the `corev1.Container` spec. After all, I think it is responsibility of the container trait to manage its own volumes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
