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

Download dependencies #123

Merged
merged 11 commits into from
Nov 30, 2020
Merged

Conversation

JShep1
Copy link

@JShep1 JShep1 commented Oct 2, 2020

Downloads all models in the <dependency> tag within a given model's metadata.pbtxt file and within the <depend> tag in a model.config. Cyclic dependencies are also allowed. A step towards getting #111 resolved.

Depends on https:/ignitionrobotics/ign-msgs/pull/91/files

Signed-off-by: John Shepherd [email protected]

Signed-off-by: John Shepherd <[email protected]>
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Oct 2, 2020
@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #123 (d97e420) into ign-fuel-tools4 (ad1967a) will increase coverage by 0.13%.
The diff coverage is 95.00%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           ign-fuel-tools4     #123      +/-   ##
===================================================
+ Coverage            77.12%   77.26%   +0.13%     
===================================================
  Files                   19       19              
  Lines                 2535     2555      +20     
===================================================
+ Hits                  1955     1974      +19     
- Misses                 580      581       +1     
Impacted Files Coverage Δ
src/FuelClient.cc 68.82% <95.00%> (+0.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad1967a...d97e420. Read the comment docs.

John Shepherd added 2 commits October 2, 2020 12:36
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 marked this pull request as ready for review October 6, 2020 19:47
@JShep1 JShep1 requested a review from nkoenig as a code owner October 6, 2020 19:47
@JShep1 JShep1 linked an issue Oct 6, 2020 that may be closed by this pull request
@JShep1 JShep1 removed a link to an issue Oct 6, 2020
@JShep1 JShep1 mentioned this pull request Oct 6, 2020
@JShep1
Copy link
Author

JShep1 commented Oct 6, 2020

Should we also add support for model.config depend tags?

@chapulina
Copy link
Contributor

Should we also add support for model.config depend tags?

I'll leave that up to @nkoenig .


Could you add some tests? Probably somewhere here: https:/ignitionrobotics/ign-fuel-tools/blob/d892487b4e99b1bb8197ccbcb4533731831de4fa/src/FuelClient_TEST.cc#L397

@JShep1
Copy link
Author

JShep1 commented Oct 7, 2020

Could you add some tests? Probably somewhere here:

Sure, I'm currently working on base case example model that I'll add to the tests once I can upload it.

@JShep1
Copy link
Author

JShep1 commented Oct 7, 2020

I added support for downloading dependencies from model.config here: #124 . We can cancel it if it's not the direction we want to go, I mainly did it to test as I can't currently upload .pbtxt files onto fuel for testing.

…_dep_download

Add support for model config dependencies
@JShep1 JShep1 marked this pull request as draft October 12, 2020 23:34
@JShep1
Copy link
Author

JShep1 commented Oct 12, 2020

Converting to draft until I can get some tests in

@JShep1 JShep1 added the needs upstream release Blocked by a release of an upstream library label Oct 13, 2020
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 marked this pull request as ready for review October 13, 2020 19:08
src/FuelClient.cc Outdated Show resolved Hide resolved
src/FuelClient.cc Outdated Show resolved Hide resolved
src/FuelClient.cc Outdated Show resolved Hide resolved
John Shepherd and others added 3 commits November 9, 2020 15:56
@nkoenig
Copy link
Contributor

nkoenig commented Nov 12, 2020

Support for model.config is fine. In the long run we should deprecate and remove model.config. We don't have a plan for that yet though.

@nkoenig
Copy link
Contributor

nkoenig commented Nov 12, 2020

I'm getting the same test failure locally as CI.

@chapulina
Copy link
Contributor

@osrf-jenkins run tests now that ign-msgs 5.4.0 is out

@JShep1 JShep1 requested a review from nkoenig November 16, 2020 19:32
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Nov 16, 2020
@JShep1
Copy link
Author

JShep1 commented Nov 23, 2020

@nkoenig Could I get a re-review? Not sure why Github isn't letting me re-request.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Nice!

@chapulina chapulina merged commit 59547e9 into ign-fuel-tools4 Nov 30, 2020
@chapulina chapulina deleted the jshep1/download_dependencies branch November 30, 2020 20:55
nkoenig added a commit that referenced this pull request Dec 9, 2020
* Use lowercase resource and owner names when storing assets on disk (#130)

* Use lowercase resource and owner names when storing assets on disk

Signed-off-by: Nate Koenig <[email protected]>

* Remove added diff text from merge

Signed-off-by: John Shepherd <[email protected]>

* change model version in test back

Signed-off-by: John Shepherd <[email protected]>

* Fix test

Signed-off-by: Nate Koenig <[email protected]>

* Fix windows

Signed-off-by: Nate Koenig <[email protected]>

* Fix more windows tests

Signed-off-by: Nate Koenig <[email protected]>

* Testing another windows fix

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: John Shepherd <[email protected]>

* Prepare for 3.5.0 release (#135)

* Prepare for 3.5.0 release

Signed-off-by: Nate Koenig <[email protected]>

* Update Changelog.md

Co-authored-by: Louise Poubel <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Download model dependencies (#123)

Signed-off-by: John Shepherd <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

* Prepare for 4.3.0 release (#139)

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

* Set keep alive on (#141)

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: John Shepherd <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
nkoenig added a commit that referenced this pull request Dec 23, 2020
* Use lowercase resource and owner names when storing assets on disk (#130)

* Use lowercase resource and owner names when storing assets on disk

Signed-off-by: Nate Koenig <[email protected]>

* Remove added diff text from merge

Signed-off-by: John Shepherd <[email protected]>

* change model version in test back

Signed-off-by: John Shepherd <[email protected]>

* Fix test

Signed-off-by: Nate Koenig <[email protected]>

* Fix windows

Signed-off-by: Nate Koenig <[email protected]>

* Fix more windows tests

Signed-off-by: Nate Koenig <[email protected]>

* Testing another windows fix

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: John Shepherd <[email protected]>

* Prepare for 3.5.0 release (#135)

* Prepare for 3.5.0 release

Signed-off-by: Nate Koenig <[email protected]>

* Update Changelog.md

Co-authored-by: Louise Poubel <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Download model dependencies (#123)

Signed-off-by: John Shepherd <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

* Prepare for 4.3.0 release (#139)

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

* Set keep alive on (#141)

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

* Support editing/patching model files (#140)

* Support editing/patching model files

Signed-off-by: Nate Koenig <[email protected]>

* Added documentation

Signed-off-by: Nate Koenig <[email protected]>

* Set keep alive on

Signed-off-by: Nate Koenig <[email protected]>

* Documentation and tests

Signed-off-by: Nate Koenig <[email protected]>

* One more test and exit catch missing -u

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

* Prepare for 5.1.0 release (#145)

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

* Fix light map URI (#146)

Signed-off-by: Ian Chen <[email protected]>

* Bump to 5.1.1 (#147)

Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: John Shepherd <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants