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

Implement repository filters #764

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mathias-luedtke
Copy link
Member

Alternative to #763

  • Update docs
  • Create test case

New syntax:

  • UPSTREAM_WORKSPACE='github:ros-controls/ros_control#melodic-devel,-rqt_controller_manager'
  • UPSTREAM_WORKSPACE='github:ros-controls/ros_control#melodic-devel,rqt_controller_manager,-rqt_controller_manager/CHANGELOG.rst
  • UPSTREAM_WORKSPACE='local_folder,-sub_folder (untested)

@rhaschke: Please have a look

Instead of "+" I used the already existing syntax for copying sub-folders.
If filter starts with "-", the filter operates on the cloned repository directly.

@mathias-luedtke mathias-luedtke force-pushed the feature/repository-filter branch from 8e94506 to e33ddb5 Compare December 29, 2021 20:20
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for your effort!
You should also consider augmenting the docs.
I noticed a minor issue when testing:
-folder1,folder2 doesn't keep folder2 only as expected (but this usage is not meaningful)

ici_import_repository "$sourcespace" "$source" "$filter"
;;
http://* | https://*) # When UPSTREAM_WORKSPACE is an http url, use it directly
ici_import_url "$sourcespace" "$source"
Copy link
Contributor

@rhaschke rhaschke Dec 29, 2021

Choose a reason for hiding this comment

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

Did you skip the filter for ici_import_url intentionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. There is no specific folder that can be filtered in this case.

.)
ici_log "Copying '$basepath'"
ici_import_directory "$sourcespace" "$basepath"
;;
/*)
Copy link
Contributor

Choose a reason for hiding this comment

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

The code here for /*) and below for *) is rather similar. Do you want to factor that out into a separate function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I will make it a fall-through

@mathias-luedtke
Copy link
Member Author

-folder1,folder2 doesn't keep folder2 only as expected (but this usage is not meaningful)

The order is important.. If the list starts with "-", the complete repo will be used (otherwise there is nothing to delete from..).
Another option would be to sort the list first or run two scans, but as you said, the semantics of your example are not very clear.

@mathias-luedtke mathias-luedtke force-pushed the feature/repository-filter branch from 13aa5ed to 979a02a Compare December 30, 2021 11:48
@mathias-luedtke mathias-luedtke force-pushed the feature/repository-filter branch from 979a02a to fd671d9 Compare December 30, 2021 12:04
@mathias-luedtke mathias-luedtke self-assigned this Jan 11, 2022
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.

2 participants