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

Add some sugar for additional sources #1400

Merged
merged 7 commits into from Apr 21, 2020

Conversation

lburgazzoli
Copy link
Contributor

Fixes #1388

Release Note

NONE

Copy link
Member

@nicolaferraro nicolaferraro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting some errors:

[nferraro@localhost camel-k]$ kamel run examples/simple.groovy --source examples/Master.java --save
integration "simple" updated
Error: open .kamel/kamel-config.yaml: permission denied

I think we've switched to a unique file in the root in order to have it clearly visible when running the integrations. I was also thinking to add a flag like --use-config to remind users that they're using some specific configurations, because I myself tend to forget..

@nicolaferraro
Copy link
Member

Also, running:

kamel run examples/simple.groovy --source examples/Master.java --source examples/petstore.groovy --save

(it doesn't make sense to run those integrations together, it's just for testing)

Followed by:

kamel run examples/simple.groovy

Should take to the same result, but configuration is not loaded in the second case.

@nicolaferraro
Copy link
Member

nicolaferraro commented Apr 14, 2020

Also, running:

kamel run examples/simple.groovy --source examples/Master.java --source examples/petstore.groovy --save

(it doesn't make sense to run those integrations together, it's just for testing)

Followed by:

kamel run examples/simple.groovy

Should take to the same result, but configuration is not loaded in the second case.

It's not specifically related to --source, also other flags are not loaded.

@lburgazzoli
Copy link
Contributor Author

@nicolaferraro mind testing it again ?

@nicolaferraro
Copy link
Member

@nicolaferraro mind testing it again ?

Still not loading

[nferraro@localhost camel-k]$ kamel run examples/hello.xml -t quarkus.enabled=true --save
integration "hello" created
[nferraro@localhost camel-k]$ kubectl get it hello -o yaml | grep quarkus
    quarkus:
  - camel-quarkus:core
  - camel-quarkus:log
  - camel-quarkus:timer
  - mvn:org.apache.camel.k/camel-k-quarkus-loader-xml
  - mvn:org.apache.camel.k/camel-k-runtime-quarkus
  runtimeProvider: quarkus
[nferraro@localhost camel-k]$ kamel delete --all
1 integration(s) deleted
[nferraro@localhost camel-k]$ kamel run examples/hello.xml
integration "hello" created
[nferraro@localhost camel-k]$ kubectl get it hello -o yaml | grep quarkus
[nferraro@localhost camel-k]$ echo "saved but not loaded"
saved but not loaded

@nicolaferraro
Copy link
Member

We should add some it tests btw

@lburgazzoli
Copy link
Contributor Author

We should add some it tests btw

there is a test but may cause the failure of other tests as we use a global viper so I disabled it

@lburgazzoli
Copy link
Contributor Author

@nicolaferraro should finally work now

@nicolaferraro
Copy link
Member

No way to override/change what it's in the kamel-config.yaml:

kamel run examples/simple.groovy --source examples/petstore.groovy --save
kamel run examples/simple.groovy --source examples/Master.java
# Still runs the first combination, ignoring Master

@lburgazzoli
Copy link
Contributor Author

No way to override/change what it's in the kamel-config.yaml:

kamel run examples/simple.groovy --source examples/petstore.groovy --save
kamel run examples/simple.groovy --source examples/Master.java
# Still runs the first combination, ignoring Master

This is a little bit tricky and not sure if it is related to this PR as in fact when the run command runs, it performs two steps:

  1. load from kamel.run
  2. load from kamel.run.integration.$name

the second step is of course not bounded to any flag as it is dynamic so any flag value gets overridden by the value from the persisted configuration.

let me think about what I can do

@lburgazzoli
Copy link
Contributor Author

so I've added a huge hack, please test it again

@nicolaferraro
Copy link
Member

nicolaferraro commented Apr 17, 2020

Mmh... save is not additive, while execution is:

kamel run examples/simple.groovy --source examples/Master.java  --save

Now in config you have:

kamel:
  run:
    integration:
      simple:
        sources:
        - examples/Master.java

Now you want to add a dependency:

kamel run examples/simple.groovy -d camel-log  --save

But what you have now in config is:

kamel:
  run:
    integration:
      simple:
        dependencies:
        - camel-log

I.e. the master integration is deleted. It's a choice, that is different from the current behavior.

But the problem is that now the integration contains both sources (simple and Master).
And if you run again kamel run examples/simple.groovy the Master source disappears.

@lburgazzoli
Copy link
Contributor Author

OK now it should be additive.

I'll revisit the whole cmd stuff after 1.0

@nicolaferraro
Copy link
Member

Looks good enough..
The only strange behavior is that it's not additive when managing lists, but it's not a huge problem.

I mean:

kamel run xxx.groovy --source yyy.groovy --save
# config contains source yyy
kamel run xxx.groovy -d camel-telegram --save
# config contains source yyy and dependency camel-telegram
kamel run xxx.groovy -d camel-class --save
# config contains source yyy and dependency camel-class (camel-telegram has gone)

@lburgazzoli
Copy link
Contributor Author

Looks good enough..
The only strange behavior is that it's not additive when managing lists, but it's not a huge problem.

Yeah, this is a know issue.

btw, looks like the CI is broken

@lburgazzoli lburgazzoli merged commit 18e3fbd into apache:master Apr 21, 2020
@lburgazzoli lburgazzoli deleted the github-1388 branch April 21, 2020 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add some sugar for additional sources
3 participants