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.

Sign up for GitHub

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

Jump to bottom

Cloud API: make location optional for Azure Upload Options #3093

Merged
merged 5 commits into from
Oct 27, 2022
Merged

Cloud API: make location optional for Azure Upload Options #3093

merged 5 commits into from
Oct 27, 2022

Conversation

thozza
Copy link
Member

@thozza thozza commented Oct 24, 2022 •
edited
Loading

Previously, the Cloud API implementation required that the client provides the Location of the Resource Group to be used for importing the image to Azure. This forced the client to provide a redundant information, that it does not necessarily have. In addition, the Resource Group Location can be easily determined using Azure API.

Let's change the implementation and make the location property optional when uploading images to Azure. If the location is not provided, osbuild-composer will deduct the appropriate location from the provided Resource Group.

Change the Cloud API test case for Azure to not set the location property when calling Cloud API.

Related to https://issues.redhat.com/browse/HMSIB-133

This pull request includes:

  • adequate testing for the new functionality or fixed issue
  • adequate documentation informing people about the change such as

@thozza thozza requested review from croissanne and ondrejbudai and removed request for croissanne October 25, 2022 06:54
@thozza
Copy link
Member Author

thozza commented Oct 25, 2022

The PR should be ready for review.

  • The c9s build issue is caused by vendor/github.com/containers/libtrust/ec_key_openssl.go:23:15: undefined: ecdsa.HashSign and should be resolved when a new version of the golang package gets to c9s repos.
  • F37 failures in CI are caused by our CI using old repo snapshots and installing old podman but new containers-common which results in crun not being installed as a dependency.

croissanne
croissanne previously approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

Perfect, thank you!

thozza added a commit to thozza/koji-osbuild that referenced this pull request Oct 25, 2022
@thozza
hub: make location optional for AzureUploadOptions
06d201c 
The Azure Resource Group Location is no longer required and
osbuild-composer can determine the correct Location from the
provided Resource Group.

Related to osbuild/osbuild-composer#3093.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
ondrejbudai
ondrejbudai requested changes Oct 25, 2022
View reviewed changes
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

Amazing! Just one small request for the API spec.

internal/cloudapi/v2/openapi.v2.yml Show resolved Hide resolved
@ondrejbudai ondrejbudai enabled auto-merge (rebase) October 25, 2022 14:49
@what-the-diff
Copy link

what-the-diff bot commented Oct 26, 2022

  • The location parameter is now optional.
  • If the location is not provided, it will be deducted from the resource group.
  • The openapi documentation was updated accordingly and a test case added to verify this change works as expected.

@thozza
internal/upload/azure: add method for getting resource group location
7798190 
Signed-off-by: Tomáš Hozza <thozza@redhat.com>
@thozza
internal/upload/azure: make location optional in various methods
833d7e0 
Make the `location` argument optional (can be now empty "") in
`RegisterImage()` and `CreateStorageAccount()` methods.

If the provided `location` argument is an empty string, then the location
is determined from the provided Resource Group instead.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
@thozza
cloudapi: make location optional in Azure Upload Options
773bbce 
Providing the `location` is no longer required for Azure Upload Options.
If it is not provided, the implementation determines the location from
the provided Resource Group. This will make the API nicer for any
client, since they won't need to provide redundant information.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
@thozza
internal/target/azure_image: mark Location as optional
a6b34ec 
Signed-off-by: Tomáš Hozza <thozza@redhat.com>
@thozza
test/api: don't provide Resource Group Location when testing Azure
1c4ee4d 
Let's default to not providing the Resource Group Location when testing
Cloud API with Azure and leave it up to the implementation to determine
the correct location to use.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
@ondrejbudai ondrejbudai merged commit 5a0d286 into osbuild:main Oct 27, 2022
@thozza thozza deleted the azure-rg-location-optional branch October 31, 2022 15:27
croissanne added a commit to osbuild/image-builder that referenced this pull request Nov 4, 2022
@croissanne
internal/v1: stop setting azure location
61b14e2 
osbuild/osbuild-composer#3093 added the ability to let the location of
the resource group be inferred by composer. Since we always passed
"eastus" by default, this prevented images being uploaded to resource
groups outside of that location.
croissanne added a commit to croissanne/image-builder that referenced this pull request Nov 4, 2022
@croissanne
internal/v1: stop setting azure location
e520bf6 
osbuild/osbuild-composer#3093 added the ability to let the location of
the resource group be inferred by composer. Since we always passed
"eastus" by default, this prevented images being uploaded to resource
groups outside of that location.
@croissanne croissanne mentioned this pull request Nov 4, 2022
Merged
croissanne added a commit to croissanne/image-builder that referenced this pull request Nov 4, 2022
@croissanne
internal/v1: stop setting azure location
2d51d4f 
osbuild/osbuild-composer#3093 added the ability to let the location of
the resource group be inferred by composer. Since we always passed
"eastus" by default, this prevented images being uploaded to resource
groups outside of that location.
croissanne added a commit to croissanne/image-builder that referenced this pull request Nov 7, 2022
@croissanne
internal/v1: stop setting azure location
af5f88e 
osbuild/osbuild-composer#3093 added the ability to let the location of
the resource group be inferred by composer. Since we always passed
"eastus" by default, this prevented images being uploaded to resource
groups outside of that location.
croissanne added a commit to osbuild/image-builder that referenced this pull request Nov 7, 2022
@croissanne
internal/v1: stop setting azure location
f1eb0ac 
osbuild/osbuild-composer#3093 added the ability to let the location of
the resource group be inferred by composer. Since we always passed
"eastus" by default, this prevented images being uploaded to resource
groups outside of that location.
thozza added a commit to thozza/koji-osbuild that referenced this pull request Nov 11, 2022
@thozza
hub: make location optional for AzureUploadOptions
58ee020 
The Azure Resource Group Location is no longer required and
osbuild-composer can determine the correct Location from the
provided Resource Group.

Related to osbuild/osbuild-composer#3093.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
ochosi pushed a commit to thozza/koji-osbuild that referenced this pull request Nov 17, 2022
@thozza @ochosi
hub: make location optional for AzureUploadOptions
319589b 
The Azure Resource Group Location is no longer required and
osbuild-composer can determine the correct Location from the
provided Resource Group.

Related to osbuild/osbuild-composer#3093.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
ondrejbudai pushed a commit to osbuild/koji-osbuild that referenced this pull request Nov 21, 2022
@thozza @ondrejbudai
hub: make location optional for AzureUploadOptions
7ccd349 
The Azure Resource Group Location is no longer required and
osbuild-composer can determine the correct Location from the
provided Resource Group.

Related to osbuild/osbuild-composer#3093.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ondrejbudai ondrejbudai ondrejbudai approved these changes

@croissanne croissanne Awaiting requested review from croissanne

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thozza @ondrejbudai @croissanne