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

Invalid URL annotation generated with binary component #2175

Closed
jeffmaury opened this issue Sep 25, 2019 · 10 comments · Fixed by #2674
Closed

Invalid URL annotation generated with binary component #2175

jeffmaury opened this issue Sep 25, 2019 · 10 comments · Fixed by #2674
Assignees
Labels
estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).
Projects

Comments

@jeffmaury
Copy link
Member

[kind/bug]

What versions of software are you using?

  • Operating System: Win10
  • Output of odo version:
odo v1.0.0-beta5 (4d1d5071)     
                                
Server: https://172.18.84.5:8443
Kubernetes: v1.11.0+d4cacc0

How did you run odo exactly?

odo create java:8 bincomp --binary target\springboot-rest2-1.0.0-SNAPSHOT.jar --project bin --app binapp

Actual behavior

The generated annotation is:

app.openshift.io/vcs-uri: 'file:///target/springboot-rest2-1.0.0-SNAPSHOT.jar'

Expected behavior

Should be:

app.openshift.io/vcs-uri: 'file:target/springboot-rest2-1.0.0-SNAPSHOT.jar'

That would allow tooling (VSCode, IntelliJ) to use this annotation to import a component from the deployment.

Any logs, error output, etc?

@mohammedzee1000
Copy link
Contributor

/kind bug
/priority high

@openshift-ci-robot openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). labels Sep 25, 2019
@amitkrout
Copy link
Contributor

The generated annotation is:
app.openshift.io/vcs-uri: 'file:///target/springboot-rest2-1.0.0-SNAPSHOT.jar'

@jeffmaury Where/how you got this annotation ?

@mohammedzee1000
Copy link
Contributor

/state in-analysis

@amitkrout
Copy link
Contributor

Screen Shot 2019-09-25 at 2 12 56 PM

Got it, It just follow the local path protocol of file:/// on windows platform because the binary source being used is a local one.

@kadel kadel added this to For consideration in Sprint 173 via automation Sep 25, 2019
@girishramnani
Copy link
Contributor

@jeffmaury do you mean file:// vs file:/// because the later means its absolute path?

@girishramnani girishramnani removed this from For consideration in Sprint 173 Sep 25, 2019
@jeffmaury
Copy link
Member Author

I mean no / at all because it's a relative URL

@girishramnani girishramnani added this to For consideration in Sprint 174 via automation Oct 14, 2019
@girishramnani girishramnani moved this from For consideration to To do in Sprint 174 Oct 14, 2019
@veillard
Copy link

ACK, it all depends on the context of the relative reference. if it's in a generated file and assuming the resource being pointed at is in the target subdirectory of where that file is placed then
target/springboot-rest2-1.0.0-SNAPSHOT.jar

but it really depends on the context of where that file containing that URI reference is located
See Section 4.1 of rfc 3986
https://www.ietf.org/rfc/rfc3986.txt

Problems: relative references are not defined for the file:// scheme , sucks isn't it :-)
the common usage is to convert file path with \ to / on Windows, try to avoid drive prefix as this finishes to screw things up !

@girishramnani girishramnani added this to For consideration in Sprint 175 via automation Nov 6, 2019
@girishramnani girishramnani removed this from To do in Sprint 174 Nov 6, 2019
@girishramnani girishramnani moved this from For consideration to To do in Sprint 175 Nov 6, 2019
@kadel kadel moved this from To do to For consideration in Sprint 175 Nov 6, 2019
@kadel kadel removed this from For consideration in Sprint 175 Nov 25, 2019
@kadel kadel added this to For consideration in Sprint 176 via automation Nov 25, 2019
@kadel kadel added estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person and removed points/3 labels Nov 27, 2019
@kadel kadel moved this from For consideration to To do in Sprint 176 Nov 27, 2019
@kadel kadel removed this from To Do in Sprint 176 Dec 16, 2019
@kadel kadel added this to For consideration in Sprint 177 via automation Dec 16, 2019
@kadel kadel moved this from For consideration to To do in Sprint 177 Dec 16, 2019
@girishramnani girishramnani added this to For concideration in Sprint 178 via automation Jan 6, 2020
@girishramnani girishramnani removed this from To Do in Sprint 177 Jan 6, 2020
@girishramnani girishramnani moved this from For concideration to To do in Sprint 178 Jan 6, 2020
@mik-dass
Copy link
Contributor

@kadel @jeffmaury @girishramnani Should this issue also include the local components?

@kadel
Copy link
Member

kadel commented Jan 27, 2020

@jeffmaury

That would allow tooling (VSCode, IntelliJ) to use this annotation to import a component from the deployment.

Do you really need this annotation for local components? The information in this annotation doesn't provide enough information to do import no?
What is the use-case for importing a component from the deployment?

To be honest I would like to get rid of this annotation for binary and local components completely.
it is called vcs-uri but we store a local path in it.

@mik-dass It would be good to investigate if we even need this annotation for local and binary components. It doesn't make much sense to store a path that is on the local system on the cluster. odo should read this path from .odo/config.yaml.
For git component, it makes sense to store it in annotations as it can be useful for other tools like developer console.

If we can get rid of app.openshift.io/vcs-uri annotations I propose the following:

  • make sure that app.openshift.io/vcs-uri is not read by odo anywhere
  • stop adding app.openshift.io/vcs-uri annotation to local and binary components
  • use app.openshift.io/vcs-uri annotation only for git component (we should be already doing it)

@girishramnani girishramnani added this to For consideration in Sprint 179 via automation Jan 27, 2020
@girishramnani girishramnani removed this from To do in Sprint 178 Jan 27, 2020
@girishramnani girishramnani moved this from For consideration to To do in Sprint 179 Jan 27, 2020
@mik-dass
Copy link
Contributor

mik-dass commented Feb 3, 2020

@mik-dass It would be good to investigate if we even need this annotation for local and binary components. It doesn't make much sense to store a path that is on the local system on the cluster. odo should read this path from .odo/config.yaml.
For git component, it makes sense to store it in annotations as it can be useful for other tools like developer console.

After some searching I found that the annotations app.openshift.io/vcs-uri is mostly used in testing, storing and displaying the path to the user. With some modification, I guess we can make the display tasks to use the local config/ context and use this only for git components as the path is anyways relative. @kadel @girishramnani

@girishramnani girishramnani added this to For consideration in Sprint 180 via automation Feb 18, 2020
@girishramnani girishramnani removed this from To do in Sprint 179 Feb 18, 2020
@girishramnani girishramnani moved this from For consideration to To do in Sprint 180 Feb 18, 2020
@mik-dass mik-dass moved this from To do to In progress in Sprint 180 Mar 5, 2020
@mik-dass mik-dass moved this from In progress to For review in Sprint 180 Mar 5, 2020
@girishramnani girishramnani removed this from For review in Sprint 180 Mar 11, 2020
@girishramnani girishramnani added this to For consideration in Sprint 181 via automation Mar 11, 2020
@girishramnani girishramnani moved this from For consideration to For review in Sprint 181 Mar 11, 2020
Sprint 181 automation moved this from For review to Done Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).
Projects
No open projects
Sprint 181
  
Done
Development

Successfully merging a pull request may close this issue.

9 participants