Skip to content

Basic configuration support#74

Merged
odinuv merged 6 commits intomasterfrom
odin-AIS-35-e
May 26, 2023
Merged

Basic configuration support#74
odinuv merged 6 commits intomasterfrom
odin-AIS-35-e

Conversation

@odinuv
Copy link
Copy Markdown
Member

@odinuv odinuv commented May 22, 2023

  • add branch_id option
  • add basic calls for working with configurations

@odinuv odinuv marked this pull request as ready for review May 22, 2023 06:59
@odinuv odinuv requested a review from pivnicek May 22, 2023 06:59
Copy link
Copy Markdown
Contributor

@pivnicek pivnicek left a comment

Choose a reason for hiding this comment

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

Looks great, just noticed a couple of things

Comment on lines +39 to +43
if not isinstance(component_id, str) or component_id == '':
raise ValueError("Invalid component_id '{}'.".format(component_id))
if not isinstance(configuration_id, str) or configuration_id == '':
raise ValueError("Invalid component_id '{}'.".format(configuration_id))
url = '{}/{}/configs/{}'.format(self.base_url, component_id, configuration_id)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want to accept empty strings? Doesn't the detail call require values?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If component_id is an empty string, it raises an error, doesn't it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

omg, sorry, I don't know how to read a simple conditional statement 🤦

Comment on lines +57 to +60
if not isinstance(component_id, str) or component_id == '':
raise ValueError("Invalid component_id '{}'.".format(component_id))
if not isinstance(configuration_id, str) or configuration_id == '':
raise ValueError("Invalid component_id '{}'.".format(configuration_id))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto

Base automatically changed from odin-AIS-35-d to master May 22, 2023 12:01
odinuv and others added 4 commits May 24, 2023 16:26
- add basic calls for working with configurations
Co-authored-by: Marc <pivnicek@users.noreply.github.com>
@odinuv odinuv mentioned this pull request May 24, 2023
Closed
@odinuv odinuv requested a review from pivnicek May 24, 2023 19:05
Copy link
Copy Markdown
Contributor

@pivnicek pivnicek left a comment

Choose a reason for hiding this comment

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

alright, lgtm

@odinuv odinuv merged commit e491a19 into master May 26, 2023
@odinuv odinuv deleted the odin-AIS-35-e branch May 26, 2023 08:11
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.

2 participants