avocado icon indicating copy to clipboard operation
avocado copied to clipboard

Assets / Cache: manually register content by_location

Open willianrampazzo opened this issue 4 years ago • 6 comments

Avocado's data cache support storing content by_location, in which a given file is referred to by its original location, meaning the same file name with a different origin location is stored on a different path in the cache.

Let's attempt to reuse that feature so that a user can point to an existing file while passing its original location and have it copied/moved to the cache under the given location. A sample use case is:

  1. User has a local file machine-firmware.bin and knows its original location at http://first.url.to.the.file
  2. User has another local file machine-firmware.bin and knows its original location at http://second.url.to.the.file
  3. While writing tests for those two different files, the user decides that these files will be referred to as machine-firmware.bin for both tests.
  4. User runs avocado asset register [--hash=VALUE] --by-location=http://first.url.to.the.file --name=machine-firmware.bin /path/to/first/machine-firmware.bin
  5. User runs avocado asset register [--hash=VALUE] --by-location=http://second.url.to.the.file --name=machine-firmware.bin /path/to/second/machine-firmware.bin

From this point on, it's possible to do, within a test:

class Test:
   def test_1(self):
      boot_image = self.fetch_asset('machine-firmware.bin', location='http://first.url.to.the.file')

   def test_2(self):
      boot_image = self.fetch_asset('machine-firmware.bin', location='http://second.url.to.the.file')

All options of fetch_asset, such as cancel_on_missing, should behave as expected.


The QEMU thread where the feature was mentioned:

https://www.mail-archive.com/[email protected]/msg786355.html

willianrampazzo avatar Mar 03 '21 18:03 willianrampazzo

Hi @willianrampazzo I understand the motivation behind this we should/could implement this.

However, currently avocado assets register accepts a name and an url as arguments, where name is a "unique name", and url is the path/location to where fetch the asset (local or remote). So, the following command is valid:

avocado assets register machine-firmware.bin http://first.url.to.the.file

The following command is also valid:

avocado assets register machine-firmware.bin /path/to/first/machine-firmware.bin

Adding a --by-location, to this command as it is, would make things not clear and confusing. IMO we should consider a bigger refactoring here.

I'm making this comment, because, as you know and I mentioned before, the entire assets logic must be re-designed. When implementing/discussing the "register/fetch by name" feature, it was not clear that name would not be unique. I'm just concerned about adding multiple "patches" to fix issues that weren't properly designed and making confusing interfaces.

IMO, we should hide from the users how we store things. Having a "by-location" or "by-name" structure should be a separate thing from what we export to the users. Because of that, maybe we should wait for the #4458 (and probably another one that I have the plan to write soon about the cache itself).

I can't see right now, how to add this feature without confusing our users with future changes. What do you think?

beraldoleal avatar Apr 27 '21 14:04 beraldoleal

I understand your point @beraldoleal and I agree a refactor is needed, but we have a request from a user. In simple words, that is

"As a user, how can I add assets with the same name and different locations manually to the cache?".

So, how can we fulfill this user's requirement? I agree the users don't need to know where the file is placed (by-name, by-location).

Or, if we think a refactor is a priority instead of the user requirement, should we say to the users that we won't add that support now because we will change the background code that manages assets?

I think the refactor is needed, but it is not a barrier to implement this request. In case others have the opposite opinion, we can wait for #4458 and tell the user it won't be implemented right now.

willianrampazzo avatar Apr 27 '21 14:04 willianrampazzo

Oh, one more comment, this use case, add an asset with the same name and different location, is valid, and Avocado will need to support it, independent of the background code.

willianrampazzo avatar Apr 27 '21 14:04 willianrampazzo

I understand the case, and I think we should support it, of course. My point is: How to implement this, without making the UI confusing for new users. Do you have a suggestion?

beraldoleal avatar Apr 27 '21 16:04 beraldoleal

I understand the case, and I think we should support it, of course. My point is: How to implement this, without making the UI confusing for new users. Do you have a suggestion?

No, I have not thought about that. I can take this item and try something.

willianrampazzo avatar Apr 27 '21 16:04 willianrampazzo

I can do it as well, I'm just pointing that having a --by-location on top of a url parameter is confusing. And changing this in future PRs could be frustrated for some users.

So unless, we design a proper solution (don't need to be a complete redesign) we are going to keep adding patches, as partial solutions, to the assets part.

We can discuss this better and try to come up with a proposal, feel free to ping me on IRC.

beraldoleal avatar Apr 27 '21 16:04 beraldoleal