OLMIS-4117 Add git to dev image

Activity

FEOLMIS-2510 16

Keyboard shortcuts  
  • Summarize the review outcomes (optional)
     
    #permalink

    Details

    Warning: no files are visible, they have all been filtered.
    Participant Role Time Spent Comments Latest Comment
    Author 39m 5 I agree, created OLMIS-4146. From my understanding of th...
    Reviewer - 0% reviewed 33m 2 Separation of jobs might help to some extent, but the Jav...
    Reviewer - 93% reviewed 37m 4 Nick Reid brought up package-lock.json files. We don't ha...
    Reviewer - Complete 20m 2 You have good points. This seems to be one of the main dr...
    Reviewer - Complete 4m 3 Sebastian Brudziński I fixed the problem
    Total   2h 14m 16  
    #permalink

    Objectives

    We have started seeing problems with local and Jenkins build. I've investigated this briefly and I'm 90% sure the reason is renamed repo for one of our transitive dependencies - https://www.npmjs.com/package/drange (from discontinuous-range). The same repo was also renamed on GitHub, although using an old URL redirects to the new one and git clones still work properly (https://github.com/fent/discontinuous-range). It seems that upon failure to find the package on npm repo, it falls back to git and then fails, since we don't have git in our dev tools.

    TL;DR:
    Are we fine adding Git to dev image?

    If we are fine with this, I'll also release v4 and update all our services.

    Branches in review

    #permalink

    Issues Raised From Comments

    Key Summary State Assignee
    #permalink

    General Comments

    Josh Zamor

    The reason we didn't include git in the dev image is it can interfere with a ...

    The reason we didn't include git in the dev image is it can interfere with a developers ".git" directory when the versions are greatly different - e.g. when the dev image has git v1 and the dev has git v40 (crazy versions for dramatic effect).

    Putting git into our dev image seems like a hack to get around an issue that exists in npm, is there another path around this?

    Chongsun Ahn

    I'm also worried that including git in the dev image may mess up git in a dev...

    I'm also worried that including git in the dev image may mess up git in a dev environment outside of docker.

    I did a quick test of removing api-spec-converter and raml-to-swagger from the main package.json and it builds locally again for me. We lose the Swagger live docs, but perhaps we could look into adding live docs using RAML tools instead? Then we don't have to do this conversion from RAML to Swagger. We might also gain more features for a small amount of effort.

    There is a ticket to move away from Swagger (https://openlmis.atlassian.net/browse/OLMIS-3708). Perhaps we could look into this for the next sprint?

    Sebastian Brudziński

    Josh Zamor: Another way would be to somehow try and exclude the problematic t...

    Josh Zamor: Another way would be to somehow try and exclude the problematic transitive dependency and manually include the correct dependency. I'm not sure if that is possible with npm (it should be) and whether it would work in the end... Also it seems like a hack to me too.

    Chongsun Ahn: Alright, I'll go ahead and remove Swagger from the remaining services for now too, so our builds pass, I'll also add OLMIS-3708 to the grooming page for the next sprint.

    Josh Zamor

    Agreed on adding 3708 to grooming of sprint 47. I've also added the ticket to...

    Agreed on adding 3708 to grooming of sprint 47. I've also added the ticket to 3.3 scope and bumped the priority to blocker.

    Paweł Gesek

    I wouldn't necessarily call this a 'hack' to get around an 'issue'. From my u...

    I wouldn't necessarily call this a 'hack' to get around an 'issue'. From my understanding a dependency that is a git repository is perfectly legal with npm - although it obviously has a lot of pitfalls. From what I understand one of the dependencies changed to this form hence the issue.

    By the way, the dev-ui image does have git installed - https://github.com/OpenLMIS/dev-ui/blob/master/Dockerfile#L8 . Wonder how many UI dependencies require it? And if it is bad to have git here, I don't see why it is ok to have it there.

    If you absolutely don't want git, a workaround is to host a tarball somewhere and override the dependency with shrinkwrap or something. Getting rid of Swagger seems a bit extreme, unless this conversion to the RAML console is something we really want to invest time in.

    Chongsun Ahn

    You have good points. This seems to be one of the main drawbacks to npm. I'm ...

    You have good points. This seems to be one of the main drawbacks to npm. I'm not sure about the dev-ui.

    I think one of the main issues here is that some of these parts in the service repo are intricately linked together. We require npm even though it has nothing to do with building Java JARs, but if it fails in some trivial way (like this example), our build is broken. Perhaps it needs to be part of a separate build job.

    We are thinking about getting rid of Swagger because there definitely seems to be problems with this API documentation generation process--it's ad hoc and has some clear limitations.

    Paweł Gesek

    Separation of jobs might help to some extent, but the Java job will still dep...

    Separation of jobs might help to some extent, but the Java job will still depend on the conversion job that uses npm, since the docs are embedded as part of the Jar. The process of building a service will also get more complex.

    As far as the RAML console goes, good news is that I see improvement since I last visited its repo (https://github.com/mulesoft/api-console) - they actually now have a working demo there plus some screenshots. Using this console definitely makes more sense than doing the conversion - I am well aware of the multiple limitations and additional complexity coming from it.

    That being said, from what I see it is a Javascript app packaged with node. So we might run into this git issue again. Is the plan to have the RAML console packaged with services (as Swagger is currently) or as a separate standalone that's part of the distro? The latter will make the jar build process no longer deal with any of the javascript/git/npm stuff.

    On a side note - how realistic is this issue with the git directory becoming incompatible? Does that happen often with git? I don't really recall many version issues between git installations.

    Josh Zamor

    Agreed with a number of the points raised here. I think a fair path forward i...

    Agreed with a number of the points raised here. I think a fair path forward is to simplify our live docs by getting rid of the conversion to swagger (OLMIS-3708).

    Overall I do think we should block a service build if the live docs fail to generate, and we'll still have API Console which uses NPM. My hope is that simplifying to stay all in RAML with a project actively maintained by Mulesoft will reduce the NPM risk to be minimal enough.

    As for not including git, in my mind it's a good idea to avoid it in the dev image, but not a hard requirement to not have it.

    Łukasz Lewczyński

    the git has been removed from dockerfile so I resolved this comment

    the git has been removed from dockerfile so I resolved this comment

    Sebastian Brudziński

    Thanks Pawel, I've removed the Swagger step from build for our services for n...

    Thanks Pawel, I've removed the Swagger step from build for our services for now, so at least they pass while we hold this debate.

    Sebastian Brudziński

    I reverted the change to docker-dev and removed Swagger convert step from the...

    I reverted the change to docker-dev and removed Swagger convert step from the builds in our services. Please note that there may still be Swagger dependencies there (I've added their removal to OLMIS-3708), this change is only to make sure builds pass.

    Josh Zamor

    Nick Reid brought up package-lock.json files. We don't have these in git, whi...

    Nick Reid brought up package-lock.json files. We don't have these in git, which seems to go against NPM recomendation. I would think we'd add these asap, and we should do that.

    Additionally I wonder if we think this would have prevented the immediate failure?

    Sebastian Brudziński

    I agree, created OLMIS-4146. From my understanding of the package locks, thi...

    I agree, created OLMIS-4146.

    From my understanding of the package locks, this would not help us. Package locks ensure that we always use the same version of the dependency (and all transitive dependencies). Here, the dependency that we were using disappeared completely (due to a rename). Most other projects continued to work properly, since if the dependency is not found in the npm registry, it falls back to git (which for GitHub redirects correctly in case of a rename).

    Łukasz Lewczyński

    Sebastian Brudziński Could you take a look on http://build.openlmis.org/job/O...

    Łukasz Lewczyński

    Sebastian Brudziński I fixed the problem

    Sebastian Brudziński I fixed the problem

    /Dockerfile Changed 1
    /build.gradle Changed
    /package.json Changed
    /build.gradle Changed
    /package.json Changed
    /build.gradle Changed
    /package.json Changed
    /build.gradle Changed
    /package.json Changed
    /build.gradle Changed
    /package.json Changed
    /build.gradle Changed
    /package.json Changed
    /build.gradle Changed
    /package.json Changed
    /tests.gradle Changed
    Open in IDE #permalink
    /build.gradle Changed
    /package.json Changed
    /build.gradle Changed
    /package.json Changed

    Review updated: Reload | Ignore | Collapse

    You cannot reload the review while writing a comment.

    Log time against