adding support for art build

[ashwini3326]

This merge provides support for generating push items from ART/rhcos builds.

[rohanpm]

This looks good, but some coverage is missing. Could you please find a way to get it to 100% coverage?

[rohanpm]

This mocking doesn't seem like it should be necessary, because you can easily point the koji source to point at any directory you want. In fact you're already pointing it at koji_dir, so you just need to write the meta.json file to the correct path under that dir and it should work without mocking.

Can you please do it that way? This way is really nasty, because:

1. You're not checking that 'join' is used correctly at all. The test can pass even if the code is totally broken and calls 'join' with incorrect arguments.

2. Mocking like this requires more maintenance in the long term, because perfectly correct changes in the code can cause the test to break just by not calling exactly the methods that you've mocked here.

[ashwini3326]

I have removed it, PTAL

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (9ab7c98) to head (1c405af).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #467   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           53        53           
  Lines         2352      2392   +40     
=========================================
+ Hits          2352      2392   +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rohanpm]

What I wrote in comment #467 (comment) applies here too. Please, don't mock open, it's completely unnecessary and it makes the test fragile as it now makes the test depend on the code-under-test calling open exactly once and only for this purpose. If someone needs to add code later which opens something else, they will be forced to rewrite this test.

There should be no reason to mock any open or path methods here. Please, check the value of the koji_dir variable while this test runs. It ought to be a directory that you can write to. Just arrange for the JSON file to really exist under that directory and then let the code under test really read it from that directory. Adding mocks unnecessarily makes the tests more complicated and fragile.

[rohanpm]

You mentioned changing this to cope with the fact that this field is optional in the schema, but this is still going to crash if the field is absent or None. Adding some test data like that could confirm it if needed.

If you want to make this tolerant against the missing field, won't it have to be written something more like this?

branch = (rhcos_meta_data.get("coreos-assembler.container-config-git") or {}).get("branch") or ""
version = branch.split("-")[-1]

[ashwini3326]

my bad I missed it, sending it again.

[ashwini3326]

I need to change the naming convention for rhcos images. I am working for it, will publish my changes once it gets completed.

[rohanpm]

I need to change the naming convention for rhcos images

@ashwini3326 could you please confirm if this PR is complete and OK to release, or are there still changes expected?

[ashwini3326]

@rohanpm After some discussion on the way we are currently naming the rhcos packages, I need to change few things and also add a 'marketplace_target' metadata for VMs'. so we need to wait before it gets merged.

ashwini said that some updates are expected.

[ashwini3326]

@rohanpm as Red Hat Core OS images are named differently ( it only have the name rhcos-) we need a way to identify where these images are expected to be shipped. I am adding a new attribute (marketplace_name) to VMIPushItem for this purpose. PTAL.

Also let me know if we need more tests for this change.

[JAVGan]

@ashwini3326 please run the tests again with PUSHSOURCE_UPDATE_BASELINES=1 and include them into this PR as the tests are failing due to missing the new attribute marketplace_name

E       AssertionError: Output differs from baseline:
E       
E       --- 
E       +++ 
E       @@ -24,6 +24,7 @@
E            ena_support: true
E            image_id: null
E            marketplace_entity_type: AMIProduct
E       +    marketplace_name: null
E            marketplace_title_template: null
E            md5sum: null
E            name: fake-image.raw
E       
E       
E       Re-run with PUSHSOURCE_UPDATE_BASELINE=1 if this is correct.

[ashwini3326]

@ashwini3326 please run the tests again with PUSHSOURCE_UPDATE_BASELINES=1 and include them into this PR as the tests are failing due to missing the new attribute marketplace_name

E       AssertionError: Output differs from baseline:
E       
E       --- 
E       +++ 
E       @@ -24,6 +24,7 @@
E            ena_support: true
E            image_id: null
E            marketplace_entity_type: AMIProduct
E       +    marketplace_name: null
E            marketplace_title_template: null
E            md5sum: null
E            name: fake-image.raw
E       
E       
E       Re-run with PUSHSOURCE_UPDATE_BASELINE=1 if this is correct.

on it.

[JAVGan]

@rohanpm just to let you know: We decided to create an attribute named marketplace-name to compose with the KojiBuildInfo.name as we thought it wouldn't be good to mess with the KojiBuildInfo.name by appending the -azure or -aws suffix to it, since the information would reflect a build which doesn't exist.

However, we need the build name to have -azure or -aws for StArMap as the RHCOS names doesn't quite follow the Stratosphere convention names, so instead we're adding these suffix to the new property and resolving it in our pubtools.