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

Migrate the Datadog::IAM::User resource to the python 3.7 runtime #91

Merged
merged 7 commits into from
Oct 21, 2020
Merged

Migrate the Datadog::IAM::User resource to the python 3.7 runtime #91

merged 7 commits into from
Oct 21, 2020

Conversation

nmuesch
Copy link
Contributor

@nmuesch nmuesch commented Oct 15, 2020 •
edited
Loading

What does this PR do?

This PR migrates the Datadog::IAM::User resource to the python 3.7 runtime. I tested this using the approach being documented here - https://github.com/DataDog/datadog-cloudformation-resources/pull/90/files

Since there are no JSON schema changes, this should be a mostly transparent migration for users of the resource

@nmuesch nmuesch requested a review from a team as a code owner October 15, 2020 22:29
@github-actions github-actions bot added the resource/user Impacts the datadog-iam-user-handler package label Oct 15, 2020
@nmuesch nmuesch added the changelog/Added Added features results into a minor version bump label Oct 15, 2020
@nmuesch nmuesch changed the title Migrate the Datadog::IAM::User resource to the python runtime Migrate the Datadog::IAM::User resource to the python 3.7 runtime Oct 16, 2020
jirikuncar
jirikuncar previously approved these changes Oct 16, 2020
View reviewed changes
zippolyte
zippolyte reviewed Oct 16, 2020
View reviewed changes
datadog-iam-user-handler/src/datadog_iam_user/version.py Outdated
@@ -0,0 +1 @@
__version__ = "2.0.0"
Copy link
Contributor

@zippolyte zippolyte Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

maybe 1.2.0 instead ? No need for major since it should be transparent to the user right ?

Suggested change
__version__ = "2.0.0"
__version__ = "1.2.0"

last released version was 1.1.0 https://github.com/DataDog/datadog-cloudformation-resources/releases/tag/datadog-iam-user-1.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, since there are no changes to the schema, I think we can do a minor. Though for now I'll just bump this to dev anyway and we can bump to the proper version at release time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
__version__ = "2.0.0"
__version__ = "1.2.0.dev"

zippolyte
zippolyte reviewed Oct 16, 2020
View reviewed changes
datadog-iam-user-handler/src/datadog_iam_user/handlers.py Outdated
model.DatadogCredentials.ApiKey,
model.DatadogCredentials.ApplicationKey,
model.DatadogCredentials.ApiURL or "https://api.datadoghq.com",
TYPE_NAME,
Copy link
Contributor

@zippolyte zippolyte Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

My mistake, let's not use TYPE_NAME directly here, and instead create a new constant with value iam-user to keep the same value as the java version

datadog-cloudformation-resources/datadog-iam-user-handler/src/main/java/com/datadog/iam/user/ClientFactory.java

Line 25 in 06c00b8

return ApiClients.V1Client(apiKey, appKey, apiURL, "iam-user", version);

I went with TELEMETRY_TYPE_NAME

zippolyte
zippolyte reviewed Oct 16, 2020
View reviewed changes
datadog-iam-user-handler/src/datadog_iam_user/handlers.py Outdated
resourceModel=model,
)

LOG.info("Starting the User Resource Create Handler")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's use the TYPE_NAME here

Suggested change
LOG.info("Starting the User Resource Create Handler")
LOG.info("Starting %s Create Handler", TYPE_NAME)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with f strings :)

zippolyte
zippolyte reviewed Oct 19, 2020
View reviewed changes
datadog-iam-user-handler/requirements.txt
@@ -0,0 +1,3 @@
cloudformation-cli-python-lib==2.1.2
git+https://github.com/datadog/datadog-api-client-python.git@750a42c58c6246d4a0a14920c438c8986a41a7bd
Copy link
Contributor

Choose a reason for hiding this comment

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

can be removed as it is a dep of common

Copy link
Contributor Author

@nmuesch nmuesch Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm going to revisit this requirements entirely once we tag a first version of the common. I'll fix both this and the monitor resource deps in a followup PR once we do that.

@nmuesch @zippolyte
Update datadog-iam-user-handler/requirements.txt
fa7c2f1 
Co-authored-by: Hippolyte HENRY <zippolyte@users.noreply.github.com>
nmuesch and others added 2 commits October 20, 2020 17:32
@nmuesch @zippolyte
Update datadog-iam-user-handler/src/datadog_iam_user/handlers.py
517589d 
Co-authored-by: Hippolyte HENRY <zippolyte@users.noreply.github.com>
@nmuesch
Address review
d43463e
@nmuesch nmuesch merged commit 817b749 into master Oct 21, 2020
@nmuesch nmuesch deleted the nick/convert_py branch October 21, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zippolyte zippolyte zippolyte approved these changes

@jirikuncar jirikuncar jirikuncar left review comments

Assignees
No one assigned
Labels
changelog/Added Added features results into a minor version bump resource/user Impacts the datadog-iam-user-handler package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nmuesch @jirikuncar @zippolyte