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

Ants 2203 migrate bamboo to airbyte #4

Merged
merged 23 commits into from
Mar 25, 2024

Conversation

NatElkins
Copy link

Updates Bamboo source for our use case.

def get_default_bamboo_fields() -> List[str]:
# As per https://documentation.bamboohr.com/docs/list-of-field-names
return [
"acaStatus"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove this as it's deprecated

Copy link
Author

Choose a reason for hiding this comment

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

My only concern is we may encounter some client with a legacy setup that is still making use of it (and not the new version, acaStatusCategory). Seems like we might want to err on the side of caution and just try to pull it in. We can choose to use it (or not) at the transform layer. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

should be fine, I think we just need to check that the deprecated fields don't result in weird bamboo endpoint error. I rmb previously when we included a deprecated field flsaCode it basically shifted all the values and keys and caused the values to be on the wrong keys in the response.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, I'll remove then. We can always add it back if someone complains (unlikely).

Copy link
Collaborator

@junquanlim junquanlim left a comment

Choose a reason for hiding this comment

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

LGTM!

@NatElkins NatElkins merged commit 119ad1e into main Mar 25, 2024
18 of 22 checks passed
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.

3 participants