Add the new attributes of releases endpoint

[xiangge]

Add the new attributes of releases endpoint

[coveralls]

Coverage remained the same at 98.874% when pulling 1669b373ea861b050c516bc8436f25142663cb6f on xiangge:add-release-feature-parity into 1b1a7ffeadd240f56bbe2ebd2f4ca461d211ce69 on product-definition-center:master.

[coveralls]

Coverage decreased (-0.5%) to 98.391% when pulling b768c8b83b66816039d0ac88a428bc39360f3222 on xiangge:add-release-feature-parity into 1b1a7ffeadd240f56bbe2ebd2f4ca461d211ce69 on product-definition-center:master.

[coveralls]

Coverage remained the same at 98.874% when pulling b768c8b83b66816039d0ac88a428bc39360f3222 on xiangge:add-release-feature-parity into 1b1a7ffeadd240f56bbe2ebd2f4ca461d211ce69 on product-definition-center:master.

[xiangge]

@romanofski as there are some columns of release endpoint just include in redhat pdc, so maybe we'd better not use the fedora PDC.

[romanofski]

@xiangge I hear you that it's a burden to change this, but I'm not very confident approving these patches which basically record internal traffic. This might be benign for this, but it basically puts a burden on every reviewer to carefully inspect the VCR records in order to not accidentally leak internal stuff. I'm not keen on having to have a talk with legal on this matter.

My preference would be: a) change every internal identifier (hostnames, products, etc) to something which is public knowledge starting with this patch b) have a convenient way to do this on recorded VCR sessions so anything which is currently in the repository is redacted

I suppose b) can be solved using with or without VCR, whatever is preferable.

[romanofski]

Ah well there is a third option now to think of it. You can also ignore my review obviously and just pick someone else who is fine to approve it.

[sthaha]

@xiangge I agree with @romanofski and the suggestions made. VCR does allow you to scrub off parts you don't want. Regarding internal information, is it feasible to record against the external / public facing pdc?

[xiangge]

@sthaha As there are some columns just in the internal pdc server like some releases endpoints' columns, so I think we can't use the public pdc server like fedora. @romanofski I agree with you that the data/url shouldn't include in the real internal data. Good point.

@sthaha and @romanofski I can mock some data in local dev db without any internal real data or just modify the original data in files.

[simozhan]

@xiangge yeah. I agree with @romanofski for the upstream project, we'd better not leak the internal information. I think it is a good idea to setup a local dev db without any internal real data. The benefit is we don't need to change some existing cases as we only change the fields, not the amount of the resources.

[simozhan]

BTW. It seems you add allowed_push_targets for release/release_variants, do you want to also add allowed_push_targets for product and product_version. Or you want to submit it in another patch

[xiangge]

@simozhan Nice catch, I added the release_variants here is because that it is related to releases testcase. and I will add this product related changes in another patch.

[simozhan]

@xiangge Ok. make sense to me.

[coveralls]

Coverage remained the same at 98.874% when pulling 6bd0ccc308dd5919654f06f244c4a6538dfa91c0 on xiangge:add-release-feature-parity into 1b1a7ffeadd240f56bbe2ebd2f4ca461d211ce69 on product-definition-center:master.

[xiangge]

Change the test data back to the original ones and just add new column here, also change the url to pdc.test.com. Please help to review again, thanks.

[coveralls]

Coverage remained the same at 98.874% when pulling 75aa9ef44cd0bdc7c2fb62a1ad941320887aac72 on xiangge:add-release-feature-parity into 1b1a7ffeadd240f56bbe2ebd2f4ca461d211ce69 on product-definition-center:master.

[xiangge]

is this patch set now supposed to be free from internal identifiers?

@romanofski thanks for the reviewing, not sure if I am understanding your question, but this patch is based on the changes of feature parity requirements on pdc server side.

[romanofski]

Dear @xiangge, my review comment explained that I'm uncomfortable with the test data in use since most of them are internal identifiers. Even if some of them are public knowledge I want to have a process in place which redacts most of those identifiers to avoid mistakenly leaking company secrets. Unless this has been address I don't feel approving this patch. If you don't share the same concerns I'm happy if you guys dismiss the review and pick a different reviewer.

Note, I think @sthaha is perhaps not easily available to review this since he moved to a different team.

[xiangge]

@romanofski yes, I have noticed that @sthaha moved to another team. You are the mainly reviewer/contributor, so your concern is important !
And you reply so fast, that's nice, thanks:).

As I didn't change the original data here, just add some null columns, let's also see other guys suggestions about the current test data (@dacdownunder @simonbaird @simozhan ).

BTW: If we should do some changes, I think we'd better to give another patch to do this, like url and identifiers changes. One idea is to change the data to some fedora's and add some missing columns manually, but for me I have no idea if we need to this change.

[simonbaird]

So what is the concern about leaking internal details? Is it the hostname? Or is it the data in the VCR file that is considered to be sensitive?

[simonbaird]

Discussed it on IRC briefly. I think the question about internal data in the VCR files is out of scope for this patch. Since there is already internal data in the VCR files, I don't see the need to prevent these from being merged.

If we want to remove the real data, it can be addressed it in future patches.

[xiangge]

@simonbaird Yes, agree, thanks for the review.

[coveralls]

Coverage remained the same at 98.874% when pulling 207407e02dd158e37c3a27d0667c53203fc58974 on xiangge:add-release-feature-parity into a6f733edf9a4adf1037a856a3b07c8203e02b81b on product-definition-center:master.