Adds to AMI Access Endpoint URL used for aws marketplace #463
Conversation
jajreidy
requested review from
rohanpm,
rbikar and
chandwanitulsi
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #463 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 53 53
Lines 2335 2344 +9
=========================================
+ Hits 2335 2344 +9 ☔ View full report in Codecov by Sentry. |
| minimum: 0 | ||
| maximum: 65536 | ||
| protocol: | ||
| enum: ["http", "https"] |
There was a problem hiding this comment.
Did you confirm the supported types are just these in some docs? Or are we just following the logic here as the endpoint must be a web one?
There was a problem hiding this comment.
I checked via the WebUI when creating a version.
There was a problem hiding this comment.
Related to my other comment about "scheme" vs "protocol", does this follow some existing API which uses "protocol"?
|
Just adding a bit of context: this will be required for some AMIs with certain products that has administrative Web pages exposed as an endpoint, like JBoss EAP. |
| port = attr.ib(type=int, validator=instance_of(int)) | ||
| """Port to access the endpoint URL.""" | ||
|
|
||
| protocol = attr.ib(type=str, validator=instance_of_str) |
There was a problem hiding this comment.
Minor: I'm not sure if you are copying some existing AWS API which already uses the term "protocol". If so, ignore this.
But in the context of a URL, this part of a URL is referred to as a "scheme", not "protocol". For example the named tuple returned by https://docs.python.org/3/library/urllib.parse.html uses "scheme". Given that this class also represents a URL, it would be nice to be consistent with that.
There was a problem hiding this comment.
Yeah, it's part of the create changeset for product versions. They still don't have a doc showing all possible inputs for it in Boto3. So we're kinda left checking their own API inputs and going from there.
src/pushsource/_impl/model/ami.py
Outdated
|
|
||
| @classmethod | ||
| def _from_data(cls, data): | ||
| """Instantiate SecurityGroup from raw dict""" |
There was a problem hiding this comment.
False copy-pasted comment, this isn't a SecurityGroup.
Deleting a comment entirely is preferred over copy-pasting an incorrect one.
src/pushsource/_impl/model/ami.py
Outdated
| default=None, | ||
| validator=optional(instance_of(AmiAccessEndpointUrl)), | ||
| ) | ||
| """Billing codes associated with this image.""" |
There was a problem hiding this comment.
Another false copy-pasted comment/doc string (this will be part of the public docs so it has to be fixed).
| minimum: 0 | ||
| maximum: 65536 | ||
| protocol: | ||
| enum: ["http", "https"] |
There was a problem hiding this comment.
Related to my other comment about "scheme" vs "protocol", does this follow some existing API which uses "protocol"?
for more information, see https://pre-commit.ci
jajreidy
force-pushed
the
add-aws-access-endpoint-url
branch
from
648f7f5 to
1614bb4
Compare
Adds to AMI ability to add access endpoint url. Used for version creation for some products in AWS Marketplace.
Refers to SPSTRAT-126