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

chore(cts): update dependency on cts generation for java #505

Merged
merged 2 commits into from
May 17, 2022
Merged

Conversation

eunjae-lee
Copy link
Contributor

🧭 What and Why

Changes included:

@netlify
Copy link

netlify bot commented May 16, 2022

Deploy Preview for api-clients-automation ready!

Name Link
🔨 Latest commit 4211808
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/62835ce53cbc430009b3c18f
😎 Deploy Preview https://deploy-preview-505--api-clients-automation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@eunjae-lee
Copy link
Contributor Author

CTS for PHP doesn't have any composer file. Is it intended? Or something we need to work on? How does it resolve the latest version of the PHP package?

@algolia-bot
Copy link
Collaborator

algolia-bot commented May 16, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.

@eunjae-lee eunjae-lee requested review from a team, damcou and millotp and removed request for a team May 16, 2022 12:31
@damcou
Copy link
Contributor

damcou commented May 16, 2022

CTS for PHP doesn't have any composer file. Is it intended? Or something we need to work on? How does it resolve the latest version of the PHP package?

Currently it uses the phpunit contained in the PHP client (require-dev dependency) :
https://github.com/algolia/api-clients-automation/blob/main/scripts/cts/runCts.ts#L23
https://github.com/algolia/api-clients-automation/blob/main/clients/algoliasearch-client-php/composer.json#L35

Is it a problem ?

@eunjae-lee
Copy link
Contributor Author

CTS for PHP doesn't have any composer file. Is it intended? Or something we need to work on? How does it resolve the latest version of the PHP package?

Currently it uses the phpunit contained in the PHP client (require-dev dependency) : https://github.com/algolia/api-clients-automation/blob/main/scripts/cts/runCts.ts#L23 https://github.com/algolia/api-clients-automation/blob/main/clients/algoliasearch-client-php/composer.json#L35

Is it a problem ?

Not a problem at all! I was just curious how it works. I think it's good because what we're trying to do here is to make sure the CTS run with the latest local version.

@@ -9,7 +9,7 @@ public abstract class CtsManager {

public abstract void addSupportingFiles(List<SupportingFile> supportingFiles);

public List<Object> getPackageDependencies() {
protected List<Object> getPackageDependencies() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was public unnecessarily.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to ask :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

As this function is only used by js it could be in the JavascriptCtsManager class only

Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

I have nothing to add on my side, I'll let @millotp review the java CTS part 👍

Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Beautiful !

@@ -9,7 +9,7 @@ public abstract class CtsManager {

public abstract void addSupportingFiles(List<SupportingFile> supportingFiles);

public List<Object> getPackageDependencies() {
protected List<Object> getPackageDependencies() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this function is only used by js it could be in the JavascriptCtsManager class only

);
}

protected void addExtraToBundle(Map<String, Object> bundle) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

so clean 💙

}

dependencies {
testImplementation 'com.algolia:algoliasearch-client-java:{{packageVersion}}-SNAPSHOT'
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you know if semver works with the SNAPSHOT suffix ? in that case we could maybe remove SNAPSHOT everywhere and just have it in the clients.config.json file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I don't remember what SNAPSHOT means in Java. Can you remind me? Is it always SNAPSHOT, or can it be something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's like experimental, it will be released on a different central repository that user have to opt in if they want to use the SNAPSHOT, so once this is out of beta the SNAPSHOT will be removed.
I read that semver accepts suffix value after dash but I'm not sure

Copy link
Collaborator

Choose a reason for hiding this comment

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

but it's fine it this stays here it works well like this, it's just a small improvement if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

semver works with ${major}.${minor}.${patch}-${WHATEVER-POSTFIX}.${number} format, which means something like 1.0.0-beta.9. Ending with SNAPSHOT but without number wouldn't work AFAIK.

@eunjae-lee eunjae-lee requested a review from millotp May 17, 2022 08:47
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Nice !

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