-
Notifications
You must be signed in to change notification settings - Fork 56
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.
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
Add replication to clouds #746
Conversation
328d7bc
to
513785e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few initial comments throughout the code.
if (c.regions) { | ||
regions = c.regions.join(" ") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's possible to autodetect here like we do for the other clouds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still waiting for a test secret to see how things go. =/. I will try as son as I got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not wait on the credentials for this. Let's leave this code in for now but also document it in docs/config.yaml
. We can post any followup fixes/improvements that we find in the coming days.
I'd like to get this PR merged sooner than later so we can start really testing it out (in the FCOS pipeline).
As part of this PR we probably need to do two other things:
|
- Add cloud replication for aws, aliyun and powervs in case it is present in the meta.json Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
- Add `--public` parameter for Aliyun cloud upload; - Update documentation to reflect the change. Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
- Rename file for accuracy; - Update files using the old libupload call. Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
- Rename parameter for accuracy; Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
- Drop code and use replicate_to_clouds function instead. Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly LGTM - a few final comments and then we should be able to push this I think.
if (c.regions) { | ||
regions = c.regions.join(" ") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not wait on the credentials for this. Let's leave this code in for now but also document it in docs/config.yaml
. We can post any followup fixes/improvements that we find in the coming days.
I'd like to get this PR merged sooner than later so we can start really testing it out (in the FCOS pipeline).
libupload.groovy
Outdated
--bucket-prefix ${c.bucket} \ | ||
--regions ${regions} \ | ||
--credentials-file \${"POWER_IMAGE_UPLOAD_CONFIG"} \ | ||
--force |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should probably remove the --force
here and in the upload_to_clouds()
function below. In the new pipeline structure we just allocate a new build ID typically.
- Remove force option in libcloud; - Update documentation about powervs regions. Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.