Skip to content

Add ref-spec matching of spaceros repository (#91). #90

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

Merged
merged 9 commits into from
Jan 11, 2024

Conversation

xfiderek
Copy link
Contributor

@xfiderek xfiderek commented Dec 17, 2023

Description

This PR fixes #91, by matching spaceros repo branch to current local branch during build. Assuming that current local branch is feature/foo, the behavior is as follows:

  • If SPACEROS_GIT_REF is not supplied by user via CLI (default behavior):
    • If feature/foo exists in spaceros repo, ros2.repos from that branch will be used
    • Otherwise file will be cloned from main branch
  • If user overrides SPACEROS_GIT_REF via CLI
    • If that branch exists in spaceros repo, that branch will be used
    • otherwise earthly returns error

It is also now possible to specify different url for spaceros git repository, which can be useful in testing scenarios, when contributors are forking spaceros repo.
If SPACEROS_REPO_URL is not supplied by user, then we use wget to clone ros2.repos file from spaceros repo. Otherwise we use Earthly's GIT CLONE command to get the file (see discussion below).

Check updated readme file for more details.

Summary of changes

  • Added logic described above to Earthfile
  • Updated README.md

How changes were tested

I have developed python file to test the changes: https://gist.github.com/xfiderek/a697dd33739c035ac6b2b1d6cdee8af1

@xfiderek xfiderek changed the title Add ref-spec matching of spaceros repository #89 Add ref-spec matching of spaceros repository (#89). Dec 17, 2023
@Bckempa Bckempa added this to the humble-2024.01.0 milestone Dec 18, 2023
@xfiderek xfiderek changed the title Add ref-spec matching of spaceros repository (#89). Add ref-spec matching of spaceros repository (#91). Dec 18, 2023
@Bckempa Bckempa self-requested a review December 18, 2023 08:46
Copy link
Contributor

@Bckempa Bckempa left a comment

Choose a reason for hiding this comment

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

Already way better than what we had and a great addition to the readme as well.

I've got a few questions but none are blockers so I'll approve after I finish checking out my local test build, but I'll hold off on hitting the merge button until you've had a chance to respond.


IF[ -z "$(git ls-remote ${SPACEROS_REPO_URL} ${GIT_BRANCH})" ]
RUN echo "branch '${GIT_BRANCH}' does not exist in spaceros repo, cloning main branch"
GIT CLONE ${SPACEROS_REPO_URL} spaceros_repo_dir/
Copy link
Contributor

Choose a reason for hiding this comment

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

Annoying we need a full clone for this but I understand why - literally the only thing I miss about SVN.

Can we at least make a third case that detects if we're just using the main branch of the default repo and uses the githubusercontent CDN to grab the file? I guess a more complete solution would be checking the url for common host platforms like GitHub and GitLab which expose files directly under a specific URL but that might be out of scope for this issue.

Copy link
Contributor Author

@xfiderek xfiderek Dec 18, 2023

Choose a reason for hiding this comment

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

These are good points. Let me break it down:

  • "Can we at least make a third case that detects if we're just using the main branch of the default repo and uses the githubusercontent CDN to grab the file?" - For default spaceros repo, I would even go one step further and say that we should use CDN whenever SPACEROS_REPO_URL="https://github.com/space-ros/space-ros.git", regardless of branch. I think that it is important, because I can imagine that if we have multiple spaceros distros, we would rarely pull from main (you would rather pull from specific tag). There is just one caveat with this approach.
  • "I guess a more complete solution would be checking the url for common host platforms like GitHub and GitLab which expose files directly under a specific URL but that might be out of scope for this issue." -> yes, but here we would ideally need to handle cases when such repos are private. Earthly's GIT CLONE handles that automagically (check earthly docs)

In summary, I would refactor that so that we use CDN whenever we pull from default spaceros repo, and use GIT CLONE otherwise. I will document that in the code so that its clear for future generations :D.

Sounds good to you @Bckempa?

Update: I've just discovered a corner case. In case the branch becomes available only after first run of Earthly, it won't pull new repos as results of "$(git ls-remote ${SPACEROS_REPO_URL} ${SPACEROS_GIT_REF})" are cached across runs. Thats where GIT CLONE command shines because it checks whether there was a new commit added and reruns the pipeline if so. Let me think about the easiest solution that will mitigate that corner case

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, you got everything I was going for and more. If this too big and needs to be split out to another ticket we can do that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I figured it out, I will post the PR today in the evening

@Bckempa
Copy link
Contributor

Bckempa commented Dec 18, 2023

Not a huge deal, but while I see it executing the feedback line:

+sources | *cached* --> RUN echo "cloning branch '${GIT_BRANCH}' from spaceros repo"

I don't see the actual echo with the substituted output. Can we find a better way to make sure it's clear in logs what branch was used?

Either way, seems to work though earthly's caching is so near magical that I want to play with it more before we merge, but you got my green checkmark.

@Bckempa
Copy link
Contributor

Bckempa commented Dec 18, 2023

Note that the space_robots CI failure is expected due to #87

@xfiderek
Copy link
Contributor Author

Not a huge deal, but while I see it executing the feedback line:

+sources | *cached* --> RUN echo "cloning branch '${GIT_BRANCH}' from spaceros repo"

I don't see the actual echo with the substituted output. Can we find a better way to make sure it's clear in logs what branch was used?

Either way, seems to work though earthly's caching is so near magical that I want to play with it more before we merge, but you got my green checkmark.

I think i will just remove these echo statements as they bring unnecessary noise. I could not find any way to log something to stdout using earthly. Is it ok with you?

@xfiderek
Copy link
Contributor Author

xfiderek commented Dec 20, 2023

Ok @Bckempa, now it should work as expected. Please take a look again.
To make sure that corner cases are covered, I ended up writing python file to make sure that all works after modifications. The file is available here. I also added you to the repository, and as soon as you accept you will be able to run this file locally.

Summary of behavior
Assuming that we are on feature/foo branch:

  • If SPACEROS_GIT_REF is not supplied by user via CLI (default behavior):
    • If feature/foo exists in spaceros repo, ros2.repos file will be cloned from feature/foo
    • Otherwise file will be cloned from main branch
  • If user overrides SPACEROS_GIT_REF via CLI
    • If that branch exists in spaceros repo, the supplied branch will be used
    • otherwise earthly returns error

Additionally, if SPACEROS_REPO_URL is not supplied by user, then we use wget to clone ros2.repos file from spaceros repo. Otherwise we use Earthly's GIT CLONE command to get the file (see discussion above).

Comment on lines +103 to +107
ARG EARTHLY_GIT_BRANCH

ARG SPACEROS_REPO_URL="https://github.com/space-ros/space-ros.git"
# if current local branch does not exist in target repo then use main. note that branch supplied from CLI overrides this behavior.
ARG SPACEROS_GIT_REF="$( [ -n \"$(git ls-remote $SPACEROS_REPO_URL $EARTHLY_GIT_BRANCH)\" ] && echo $EARTHLY_GIT_BRANCH || echo 'main' )"
Copy link
Contributor Author

@xfiderek xfiderek Dec 20, 2023

Choose a reason for hiding this comment

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

As we want to re-run git ls-remote every time, we can no longer keep these args at the top of the file

@Bckempa Bckempa linked an issue Dec 27, 2023 that may be closed by this pull request
@EzraBrooks
Copy link
Member

looks like this should be good to go after a rebase

@xfiderek xfiderek force-pushed the feat/refspec-match branch from 63cf59c to 21a7704 Compare January 2, 2024 07:14
@xfiderek
Copy link
Contributor Author

xfiderek commented Jan 2, 2024

Rebase applied

@Bckempa Bckempa force-pushed the feat/refspec-match branch from 21a7704 to 3790bdc Compare January 11, 2024 19:11
@Bckempa Bckempa merged commit 5c0c061 into space-ros:main Jan 11, 2024
eholum pushed a commit to eholum/docker that referenced this pull request Jun 9, 2024
eholum pushed a commit to eholum/space-ros that referenced this pull request Jun 26, 2024
eholum pushed a commit to eholum/space-ros that referenced this pull request Jun 26, 2024
eholum pushed a commit to eholum/space-ros that referenced this pull request Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

docker/spaceros does not build Earthly should ref-spec match Space ROS repos during build
3 participants