Uploaded image for project: 'Bigtop'
  1. Bigtop
  2. BIGTOP-1527

Allow to fetch package's source code from Git

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 1.0.0
    • Component/s: general
    • Labels:
      None

      Description

      For development purposes. It's useful to target Bigtop on Git-repository of your own package.

        Issue Links

          Activity

          Hide
          cos Konstantin Boudnik added a comment -

          Could you please take a look at BIGTOP-1373 to see if this is relevant to what you're trying to do?

          Show
          cos Konstantin Boudnik added a comment - Could you please take a look at BIGTOP-1373 to see if this is relevant to what you're trying to do?
          Hide
          ivanorlov Ivan Orlov added a comment -

          Thank you, I've not seen BIGTOP-1373.

          But I think that my improvement request/patch is an ad-hoc solution for that task. My patch is subset of BIGTOP-1373, so I hope that mine could be applied before BIGTOP-1373.

          Do you see any problems in my patch, I would be happy to fix them.

          Show
          ivanorlov Ivan Orlov added a comment - Thank you, I've not seen BIGTOP-1373 . But I think that my improvement request/patch is an ad-hoc solution for that task. My patch is subset of BIGTOP-1373 , so I hope that mine could be applied before BIGTOP-1373 . Do you see any problems in my patch, I would be happy to fix them.
          Hide
          cos Konstantin Boudnik added a comment - - edited

          Thanks for working on that!
          And yeah, I think this could be done independently from the other one. A couple of technical points:

          • for cloning, could you look if gradle-git plugin can be used instead of doing exec? Similarly to how the current download is done via a plugin?
          • here it makes sense to check if GIT_BRANCH is already set
            +    if (GIT_REPO) {
            
          Show
          cos Konstantin Boudnik added a comment - - edited Thanks for working on that! And yeah, I think this could be done independently from the other one. A couple of technical points: for cloning, could you look if gradle-git plugin can be used instead of doing exec? Similarly to how the current download is done via a plugin? here it makes sense to check if GIT_BRANCH is already set + if (GIT_REPO) {
          Hide
          ivanorlov Ivan Orlov added a comment -

          Patch that uses grgit gradle plugin

          Show
          ivanorlov Ivan Orlov added a comment - Patch that uses grgit gradle plugin
          Hide
          ivanorlov Ivan Orlov added a comment -

          There is one drawback point about GrGit. It doesn't allow to set clone depth, so causes additional traffic/time.
          Any way that approach seems for me more appropriate. Thanks for hint.

          Show
          ivanorlov Ivan Orlov added a comment - There is one drawback point about GrGit. It doesn't allow to set clone depth, so causes additional traffic/time. Any way that approach seems for me more appropriate. Thanks for hint.
          Hide
          cos Konstantin Boudnik added a comment - - edited

          Thanks Ivan. A few comments:

          • would be good to add a HowTo not to README.md on how to use this new feature. While I can guess the meaning of GIT_REPO and GIT_REF there's no way a new developer can figure out on its own that such feature exists. So, add a section a "For Developers: Building a component from Git repo" to the end of the file.
          • I did these changes to the datafu component
             DATAFU_TARBALL_SRC=v$(DATAFU_BASE_VERSION).tar.gz
            -DATAFU_SITE=https://github.com/linkedin/datafu/archive
            -DATAFU_ARCHIVE=$(DATAFU_SITE)
            +DATAFU_GIT_REPO=git@github.com:linkedin/datafu.git
            +DATAFU_GIT_REF=1.0.0
            +DATAFU_ARCHIVE=$(DATAFU_GIT_REPO)
             $(eval $(call PACKAGE,datafu,DATAFU))
            

            and tried to build it. While cloning had happened I see that the resulting tarball contains .git directory in it.

          • also, the commit message should be in the form of "BIGTOP-1527. JIRA title"
          • if cloning fails for whatever reason, the temp directory is left behind under dl/ directory. Which will fail the next run. Perhaps, it makes sense to make sure the temp.dir is removed before cloning?

          As for the missing depth: perhaps we shall offer a fix to jgit?

          Show
          cos Konstantin Boudnik added a comment - - edited Thanks Ivan. A few comments: would be good to add a HowTo not to README.md on how to use this new feature. While I can guess the meaning of GIT_REPO and GIT_REF there's no way a new developer can figure out on its own that such feature exists. So, add a section a "For Developers: Building a component from Git repo" to the end of the file. I did these changes to the datafu component DATAFU_TARBALL_SRC=v$(DATAFU_BASE_VERSION).tar.gz -DATAFU_SITE=https: //github.com/linkedin/datafu/archive -DATAFU_ARCHIVE=$(DATAFU_SITE) +DATAFU_GIT_REPO=git@github.com:linkedin/datafu.git +DATAFU_GIT_REF=1.0.0 +DATAFU_ARCHIVE=$(DATAFU_GIT_REPO) $(eval $(call PACKAGE,datafu,DATAFU)) and tried to build it. While cloning had happened I see that the resulting tarball contains .git directory in it. also, the commit message should be in the form of " BIGTOP-1527 . JIRA title" if cloning fails for whatever reason, the temp directory is left behind under dl/ directory. Which will fail the next run. Perhaps, it makes sense to make sure the temp.dir is removed before cloning? As for the missing depth : perhaps we shall offer a fix to jgit?
          Hide
          cos Konstantin Boudnik added a comment -

          Linking the issue in JGit, that asks for shallow copies.

          Show
          cos Konstantin Boudnik added a comment - Linking the issue in JGit, that asks for shallow copies.
          Hide
          ivanorlov Ivan Orlov added a comment -

          Added a new patch-file, consider your remarks.

          Show
          ivanorlov Ivan Orlov added a comment - Added a new patch-file, consider your remarks.
          Hide
          rvs Roman Shaposhnik added a comment -

          +1 on the new patch! Konstantin Boudnik any chance you can do a final review and commit?

          Show
          rvs Roman Shaposhnik added a comment - +1 on the new patch! Konstantin Boudnik any chance you can do a final review and commit?
          Hide
          cos Konstantin Boudnik added a comment -

          Thanks Ivan Orlov, appreciate the push. BTW, you don't need to remove previous patches from the JIRA as they might be good to have for future revisiting of the patch history.
          Instead just keep adding them under the same name and JIRA will sort them out properly, so you always know which one is the latest.

          A minor comment (not worthy redoing the patch though)

          • you don't need dir == null as just above you had {{if (GIT_REPO && GIT_REF) }}

          +1 I will commit it shortly.

          Show
          cos Konstantin Boudnik added a comment - Thanks Ivan Orlov , appreciate the push. BTW, you don't need to remove previous patches from the JIRA as they might be good to have for future revisiting of the patch history. Instead just keep adding them under the same name and JIRA will sort them out properly, so you always know which one is the latest. A minor comment (not worthy redoing the patch though) you don't need dir == null as just above you had {{if (GIT_REPO && GIT_REF) }} +1 I will commit it shortly.
          Hide
          cos Konstantin Boudnik added a comment -

          Committed and pushed as
          10193c8..b165d0c HEAD -> master

          Thanks Ivan!

          Show
          cos Konstantin Boudnik added a comment - Committed and pushed as 10193c8..b165d0c HEAD -> master Thanks Ivan!
          Hide
          ivanorlov Ivan Orlov added a comment -

          I didn't get this

          > you don't need dir == null as just above you had {{if (GIT_REPO && GIT_REF) }}

          `_GIT_DIR` is optional so we need to check `dir` for null. Thanks/

          Show
          ivanorlov Ivan Orlov added a comment - I didn't get this > you don't need dir == null as just above you had {{if (GIT_REPO && GIT_REF) }} `_GIT_DIR` is optional so we need to check `dir` for null. Thanks/
          Hide
          cos Konstantin Boudnik added a comment -

          Never mind - my brain is still on vacation: I obviously was looking at the wrong variable GIT_REF. Thanks for keeping me up-right.

          Show
          cos Konstantin Boudnik added a comment - Never mind - my brain is still on vacation: I obviously was looking at the wrong variable GIT_REF. Thanks for keeping me up-right.

            People

            • Assignee:
              ivanorlov Ivan Orlov
              Reporter:
              ivanorlov Ivan Orlov
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development