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

add basic appstream support #420

Closed
wants to merge 2 commits into from
Closed

Conversation

btkostner
Copy link
Contributor

So this is a basic implementation for appstream support #384
This is my first golang code so please forgive any bad practices.

This expects you to have a file path similar to the setup repository with uncompressed appstream files like such: /var/appstream/stable/dists/xenial/main/Components-amd64.yml. You would then set the appStreamDir field in the aptly configuration file to /var/appstream. On publish aptly would then compress the files as needed, and use them in Release file.

More improvements:

  • use the compressed files outputted by asgen instead of requiring the uncompressed version.

Copy link
Contributor

@smira smira left a comment

Choose a reason for hiding this comment

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

Thanks, I feel positive overall about this change.

Several things in addition to comments:

  1. Looks like PR should be rebased
  2. We need some sorted of system (functional) tests which would cover this new feature
  3. We need some docs

@@ -22,6 +22,7 @@ gom 'github.com/smira/lzma', :commit => '7f0af6269940baa2c938fabe73e0d7ba4120568
gom 'github.com/golang/snappy', :commit => '723cc1e459b8eea2dea4583200fd60757d40097a'
gom 'github.com/syndtr/goleveldb/leveldb', :commit => '917f41c560270110ceb73c5b38be2a9127387071'
gom 'github.com/ugorji/go/codec', :commit => '71c2886f5a673a35f909803f38ece5810165097b'
gom 'github.com/ulikunitz/xz', :commit => 'f852725cf3ed4e7dca9bd458fe881ff02b27eae4'
Copy link
Contributor

Choose a reason for hiding this comment

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

This library says it's "alpha quality", at the same time we have xz decompression via github.com/smira/go-xz (which is using external process).

Can we keep one or another, or completely skip .xz for now? (until Go support becomes more mature?)

compressable bool
onlyGzip bool
ignoreFlat bool
compressOnly bool
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between ignoreFlat and compressOnly ?

@@ -95,6 +104,10 @@ func (file *indexFile) Finalize(signer utils.Signer) error {
}

for _, ext := range exts {
if file.compressOnly && !(ext == ".gz" || ext == ".bz2" || ext == ".xz") {
Copy link
Contributor

Choose a reason for hiding this comment

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

if ignoreFlat removes empty ext, what does this check do?

@@ -452,6 +454,36 @@ func (p *PublishedRepo) GetLabel() string {
return p.Label
}

// GetAppStream returns full path list of appstream files
func (p *PublishedRepo) GetAppStream(component string) ([]string, error) {
if utils.Config.AppStreamDir == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

The way aptly is implemented, it is supposed that Config object is not accessed from this level.

I would suggest two changes:

  • push AppStreamDir to Context
  • pass AppStreamDir to this method as parameter (same below in publish.go).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Publish function below will call the GetAddonFiles method. Should I pass the AppStreamDir all the way down from the API and CLI calls?

@btkostner btkostner mentioned this pull request Jan 12, 2017
2 tasks
@btkostner
Copy link
Contributor Author

Closing in favor of #473

@btkostner btkostner closed this Jan 12, 2017
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