Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

repo manifest upload (reworked) #1440

Merged
merged 3 commits into from
Nov 11, 2019
Merged

Conversation

lbonn
Copy link
Contributor

@lbonn lbonn commented Nov 11, 2019

New version of #1438, with a custom written conversion routine.

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
@lbonn lbonn changed the title repo manifest uploade (reworked) repo manifest upload (reworked) Nov 11, 2019
pattivacek
pattivacek previously approved these changes Nov 11, 2019
Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

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

I'm slightly amused that you ended up writing your own converter in the end, but it's really tight and looks like it should do the job, so if it works, it's fine with me. I like using the boost xml parser.

@doanac
Copy link
Collaborator

doanac commented Nov 11, 2019

Two small things you might want to consider. We've encountered both at Foundries and have resorted to always using the repo tool to tell us the content of the manifest:

  1. You can have more than one .xml file. In fact, that's now the way we tell people to build against our LMP. Take our manifest, then create your own "overrides" file which can customize your own layers.
  2. Not everyone uses a "pinned manifest", so the contents of the file aren't always precise. You can have repo generate a pinned manifest with something like repo manifest -r -o $archive/manifest.pinned.xml

I suppose the solution to the issue I'm raising is to simply generate this file myself and then pass that to this command. However, it might make things easier for users if this command could do the dirty work for them.

Using boost and jsoncpp

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
@lbonn
Copy link
Contributor Author

lbonn commented Nov 11, 2019

Two small things you might want to consider. We've encountered both at Foundries and have resorted to always using the repo tool to tell us the content of the manifest:

1. You can have more than one .xml file. In fact, that's now the way we tell people to build against our LMP. Take our manifest, then create your own "overrides" file which can customize your own layers.

2. Not everyone uses a "pinned manifest", so the contents of the file aren't always precise. You can have repo generate a pinned manifest with something like `repo manifest -r -o $archive/manifest.pinned.xml`

I suppose the solution to the issue I'm raising is to simply generate this file myself and then pass that to this command. However, it might make things easier for users if this command could do the dirty work for them.

If I understand your point correctly, this is already what we are planning to do. The input manifest is generated in this bbclass before being passed to garage-push: https://github.com/advancedtelematic/meta-updater/blob/21a463dc264434dc132ae79526a824e7955e0032/classes/image_repo_manifest.bbclass#L17.

Nevertheless, I hadn't thought about a multi-manifest case. That sounds interesting and we might recommend that as well if it works as expected.

@doanac
Copy link
Collaborator

doanac commented Nov 11, 2019

@pattivacek
Copy link
Collaborator

You can have more than one .xml file. In fact, that's now the way we tell people to build against our LMP. Take our manifest, then create your own "overrides" file which can customize your own layers.

Interesting, can you share a link or explain how that works? And is it possible to merge the manifests in some way for export, or how would you suggest keeping track of that?

@doanac
Copy link
Collaborator

doanac commented Nov 11, 2019

Interesting, can you share a link or explain how that works?

The way we manage it is like this: Each custom forks our lmp-manifes. They then create a their own "overrides" file. For example I have something like this:

# andy-corp.xml
<manifest>
  <include name="default.xml" />

  <remote fetch="https://source.foundries.io/partners/andy-corp"
          name="subscriber-overrides" />
  <remote fetch="https://github.com/doanac"
          name="gh" />

  <remove-project name="meta-lmp" />
  <project name="meta-lmp" path="layers/meta-lmp" remote="gh" revision="8608f34dc2c9255839466e73970445e03f220efe" />

  <project name="meta-subscriber-overrides"
           path="layers/meta-subscriber-overrides"
           remote="subscriber-overrides"/>
</manifest>

To initialize my repository I then run something like: repo init -u https://source.foundries.io/factories/andy-corp/lmp-manifest.git -m andy-corp.xml

This is super handy because you can see that I can override and add things to the common platform to make it "my own". And the repo manifest -r -o $archive/manifest.pinned.xml understands that and produces a single combined file reduced to the proper content.

The huge benefit is that its pretty easy to merge our changes back into a forked repo without merge conflicts since they are doing thing in their own file.

@codecov-io
Copy link

codecov-io commented Nov 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@3515a2f). Click here to learn what that means.
The diff coverage is 67.69%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1440   +/-   ##
=========================================
  Coverage          ?   80.35%           
=========================================
  Files             ?      183           
  Lines             ?    11011           
  Branches          ?        0           
=========================================
  Hits              ?     8848           
  Misses            ?     2163           
  Partials          ?        0
Impacted Files Coverage Δ
src/sota_tools/garage_push.cc 60.86% <13.04%> (ø)
src/libaktualizr/utilities/xml2json.h 97.61% <97.61%> (ø)

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 3515a2f...4a1779e. Read the comment docs.

@lbonn
Copy link
Contributor Author

lbonn commented Nov 11, 2019

Fixed formatting + small lint and added some basic exception handling.

@pattivacek
Copy link
Collaborator

This is super handy because you can see that I can override and add things to the common platform to make it "my own". And the repo manifest -r -o $archive/manifest.pinned.xml understands that and produces a single combined file reduced to the proper content.

The huge benefit is that its pretty easy to merge our changes back into a forked repo without merge conflicts since they are doing thing in their own file.

Cool, thanks! And as I was hoping, if repo manifest -o understands it, then it will be compatible with our manifest upload strategy.

@lbonn lbonn merged commit 9677f02 into master Nov 11, 2019
@lbonn lbonn deleted the feat/OTA-3883/repo-manifest-upload-2 branch November 11, 2019 16:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants