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

[VEG-2395] axios fetch adapter #253

Merged
merged 30 commits into from
Sep 5, 2024
Merged

[VEG-2395] axios fetch adapter #253

merged 30 commits into from
Sep 5, 2024

Conversation

anushkafka
Copy link
Contributor

@anushkafka anushkafka commented Aug 28, 2024

Description

dced827 fix create part 1

af15f40 fix create part 2

746775e fix update

535f22f update

1468f51 working apps package tests

8b2a1d6 working apps validate func tests

bd824e7 Working themes delete tests

959fe54 working import tests

b3cac14 working themes list tests

9993eaa working themes publish tests

cddd8ce fix themes preview func tests

7a01ce5 working request unit tests

c07dc9e restore package.json

087e0cb Fix lint

9741a0c wip

eacda19 loginInteractively test fix wip

8698cd8 auth tests wip

904f5dd working auth tests

3980774 Restore package.json original test scripts

3eb9e72 Remove nock from the main package

4c895f3 Update form data

7fb536d Fix linting

9c8d179 Tidy up test file

2586471 Update tests after rebase

cf3d908 Themes change wip

f2a6f18 Working themes code

5377c27 Clean up code

48d5ea1 Adjust themes tests

b67950b Fix linting

6ece334 Clean up

Detail

This card requests the use of fetch instead of axios to make API calls.
To accomodate this request I have opted to use the new fetch adapter available through axios instead of removing it entirely due to the comparatively lower code changes to the core functionality.

A few things to note,

  • I have bumped axios to access the fetch adapter and fetch unfortunately does not support nock for tests.
  • I have fixed an issue with the use of the Archiver NPM module. It seems like we have misinterpreted the use of archive.finalize(). It does not indicate the stream is finished. Here is an excerpt from the documentation finalize the archive (ie we are done appending files but streams have to finish yet). 'close', 'end' or 'finish' may be fired right after calling this method so register to them beforehand. I believe this change set contains the proper use of archiver.

Jira: https://zendesk.atlassian.net/browse/VEG-2395

Checklist

  • 💂‍♂️ includes new unit and functional tests

@anushkafka anushkafka changed the title Af/axios fetch adapter [AF-2395] axios fetch adapter Aug 28, 2024
@anushkafka anushkafka changed the title [AF-2395] axios fetch adapter [VEG-2395] axios fetch adapter Aug 28, 2024
@anushkafka anushkafka marked this pull request as ready for review August 29, 2024 07:00
@anushkafka anushkafka requested review from a team as code owners August 29, 2024 07:00
Copy link
Contributor

@luis-almeida luis-almeida left a comment

Choose a reason for hiding this comment

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

Thanks for all this work! 🙏🏼
👍🏼 for the "themes" changes.

Copy link

@EmilyBie EmilyBie left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a minor unblocking question.

Copy link
Contributor

@JeanMarcGoepfert JeanMarcGoepfert left a comment

Choose a reason for hiding this comment

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

Not sure exactly what the issue is, but I'm seeing the following after a quick test:

When running the following on master:
ZENDESK_SUBDOMAIN=z3n-jgoepfert ZENDESK_EMAIL=jgoepfert@zendesk.com ZENDESK_API_TOKEN=my_api_key yarn dev apps:package test_app

I get a successful response:

Package created at test_app/tmp/app-20240902003558758.zip

On the af/axios-fetch-adapter branch, I get:

 ›   Error: AxiosError: Request failed with status code 500
error Command failed with exit code 2.

@anushkafka anushkafka force-pushed the af/axios-fetch-adapter branch from c23b556 to 9c8d179 Compare September 4, 2024 00:28
@anushkafka
Copy link
Contributor Author

anushkafka commented Sep 4, 2024

@luis-almeida I have had to make a few changes to the themes code base with regards to file operations (in the last few commits) Happy to address nits but a little hesitant to do more. I have tested this change with themes import and publish with the copenhagen theme. If it looks alright it would be great if you can do a QA for themes.

Screenshot 2024-09-04 at 3 27 08 PM

Copy link
Contributor

@JeanMarcGoepfert JeanMarcGoepfert left a comment

Choose a reason for hiding this comment

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

Confirmed the issue I was seeing previously is no longer happening.

Also tested all the other apps commands with credentials provided via env vars and profile - all works fine. Login/profile commands all work fine too. So all good from my end once tests are passing.

@luis-almeida Any manual testing you'd like to do on your end considering there are a few changes to theme related code here?

@anushkafka anushkafka requested a review from a team September 4, 2024 07:19
Copy link
Contributor

@luis-almeida luis-almeida left a comment

Choose a reason for hiding this comment

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

@anushkafka @JeanMarcGoepfert I manually tested all the theme commands and it all looks good!
Didn't test with all the supported env vars but we have good unit and functional test coverage so we should've seen it fail if there was any issue there.

All code changes look good and make sense so nothing to point out 👍🏼
Only nit would be to squash for a cleaner history since it's all related.

Copy link

@EmilyBie EmilyBie left a comment

Choose a reason for hiding this comment

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

I've reviewed the code changes, and everything looks good to me 👍. Great job on the fix!

Just a friendly suggestion: you might want to consider using Semantic Commit Messages and git fixup & autosquash for maintaining a cleaner branch history in your future PRs.

@anushkafka anushkafka merged commit ce8c221 into master Sep 5, 2024
4 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.

4 participants