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.

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

Fix: racially insensitive language #205

Merged
merged 12 commits into from Mar 9, 2021
Merged

Fix: racially insensitive language #205

merged 12 commits into from Mar 9, 2021

Conversation

aspeake
Copy link
Contributor

@aspeake aspeake commented Jan 13, 2021

Fixes #164

Pull Request Description

Removes racially insensitive language for EMR instance types, updating the following arguments:

  • master_instance_type ==> manager_instance_type
  • slave_instance_type ==> worker_instance_type
  • slave_instance_count ==> worker_instance_count

Also, now fixes the broken AWS workflow.

Checklist

Not all may apply

  • Code changes (must work)
  • Tests exercising your feature/bug fix (check coverage report on CircleCI build -> Artifacts)
  • All other unit tests passing
  • Update validation for project config yaml file changes
  • Update existing documentation
  • Run a small batch run to make sure it all works (local is fine, unless an Eagle specific feature)
  • Add to the changelog_dev.rst file and propose migration text in the pull request

@aspeake aspeake requested a review from nmerket January 13, 2021 18:45
Copy link
Member

@nmerket nmerket left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good. We might want to wait a bit to merge this because of input schema changes. I would feel more comfortable if we could test this somehow as well.

Comment on lines 36 to 38
master_instance_type: str(required=False)
slave_instance_type: str(required=False)
slave_instance_count: int(min=1, required=False)
manager_instance_type: str(required=False)
worker_instance_type: str(required=False)
worker_instance_count: int(min=1, required=False)
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be changing old schemas as the only function they serve now is to keep a record of previous input formats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, addressed in 517049d

Comment on lines 38 to 40
master_instance_type: str(required=False)
slave_instance_type: str(required=False)
slave_instance_count: int(min=1, required=False)
manager_instance_type: str(required=False)
worker_instance_type: str(required=False)
worker_instance_count: int(min=1, required=False)
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Since we're making breaking changes to the input format, we should make a v0.3 schema and upgrade to that. I actually have a v0.3 schema going on in #187. Maybe we should wait for that to merge (soon) and make these changes there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 517049d. That sounds good, we can hold off for the next schema to get merged.

@nmerket nmerket merged commit 9459728 into develop Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove racially insensitive language
2 participants