squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-999536039
> > > > 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 is 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. > > Hope it answer your question, I'm not entirely sure we're talking about the same thing here :) > > I think we are talking about the same thing :) But that is general description, that I still fail to reconcile with the actual implementation :) Maybe this could be cleared if you could help me understand: > > * Why the file information leaks into the _container_ trait configuration, following the comment ` Syntax: [configmap|secret|file]:name[key], where name represents the local file path or the configmap/secret` and my interpretation of the current implementation? > Ah, this is a mistake. I copied the description from the run command. In the trait we cannot have a `file` but only `configmap|secret`. The run is in charge to convert a file into the related autogenerated configmap, ie , `-t container.resource=configmap:cm-xyz`. > * Also why the generated ConfigMap name is hashed based on local information? Could we mimic what `kubectl create configmap` does? > Originally I used the file name as a configmap name. But that leads to 2 problems: 1. We are not able to provide the same file name in the namespace, ie, `kamel run -n i1 -r file:test.txt` and `kamel run -n i2 -r file:test.txt` as the second would reuse the content of the previously created configmap 2. We are not able to detect easily a change in the content, as we need to inspect the content of the configmaps when `--sync` is enabled Using an hash solves both problems. > > I'm trying to understand if the consistency of the Integration configuration being created is still satisfied, meaning if user A create an integration with a local file resource, user B should still be able to operate the Integration. I hope that makes sense :) > > > > > > 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 pattern 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 I'm trying to determine here, aren't we about to duplicate the garbage collection that we already have for resources generated by the operator. I get the particularity of the "local file" use case, but would there be a way to make it fit into the existing garbage collection mechanism. > Yes, reason why I'd keep the generated content until the Integration lives. > > > > > 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. > > Yes, that is also my understanding of the technical details. I'm trying to reason at the functional level, from the end-user standpoint. Definitely, the usage of `container` is driven by the fact that, technically speaking, the volumes belong to the Container. As we had the option only in the `kamel run`, so far, we did not expose that kind of information. If we move into a trait it becomes public, so, we may think to move into maybe a `volume` trait. Or maybe a `resource` trait. -- 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: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
