Skip to content

[ENG-916] update gem from new oas - #15

Merged
glebedel merged 16 commits into
StackOneHQ:mainfrom
glebedel:ENG-916/update-gem-from-new-oas
Aug 30, 2023
Merged

[ENG-916] update gem from new oas#15
glebedel merged 16 commits into
StackOneHQ:mainfrom
glebedel:ENG-916/update-gem-from-new-oas

Conversation

@glebedel

@glebedel glebedel commented Aug 23, 2023

Copy link
Copy Markdown
Contributor

looks like our OAS had significant changes since the last time we generated this gem (additional endpoints and more importantly change of model/entity names)

I used the openapi generator after bumping the gem version to 1.7 in the generator config. I then manually:

  • updated the base URLs as they seem to be wrongly generated.
  • restored retry logic (as it's custom)
  • restored API key auth field vs username/password

@swarmia

swarmia Bot commented Aug 23, 2023

Copy link
Copy Markdown

@Desocode Desocode left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM


module StackOneHRIS
# The employment work schedule type (e.g., full-time, part-time)
class EmploymentEmploymentContractType

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.

The names right now are not consistent, maybe we need to take a look at the OAS to make improve this.

require 'time'

module StackOneHRIS
class HrisCreateEmployeeRequestDto

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.

This should not probably be named in the lib Dto

:'zip_code' => :'String',
:'country' => :'CountryCodeEnum',
:'location_type' => :'LocationTypeEnum'
:'country' => :'LocationCountry',

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.

All the enum were named with the postfix Enum and were changed? 🤔

@jrebocho jrebocho left a comment

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.

The changes LGTM and they don't seem to break any of the existing use cases. There are some changes that we need to review in the OAS, it seems that we are using different naming styles and conventions that should be addressed like naming enums or DTOs.

@glebedel
glebedel merged commit b5f1b91 into StackOneHQ:main Aug 30, 2023
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