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

Automatically add next versions #40

Merged
merged 1 commit into from Feb 8, 2024
Merged

Conversation

Hlavtox
Copy link
Contributor

@Hlavtox Hlavtox commented Feb 3, 2024

Replacement of #39.

Questions Answers
Description? Automatically creates endpoint for next major, patches of current major and patches for previous major.
Type? refactor
BC breaks? no
Deprecations? no
Fixed ticket?
Sponsor company
How to test?

Before:
1.7.8.10
8.0.4
8.0.5
8.1.3
8.1.4
9.0.0

After:
1.7.8.10
8.0.4
8.0.5
8.1.3
8.1.4
9.0.0
10.0.0 // Added
9.0.1 // Added
9.1.0 // Added
8.1.5 // Added
8.2.0 // Added

@Hlavtox Hlavtox added the integration-deployment To deploy the assets to the integration storage label Feb 3, 2024
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 3, 2024

@matks @jolelievre Added tests to test getting version numbers. Added tests to test getting next major, minor and patch versions. Moved the PR into upstream.

Can you tell me how can I now test the deployment? https://preprod-api.prestashop-project.org/modules/8.2.0 still doesn't contain nothing. 🤷‍♂️

@Hlavtox Hlavtox added integration-deployment To deploy the assets to the integration storage and removed integration-deployment To deploy the assets to the integration storage labels Feb 3, 2024
@matks
Copy link
Contributor

matks commented Feb 4, 2024

@matks @jolelievre Added tests to test getting version numbers. Added tests to test getting next major, minor and patch versions. Moved the PR into upstream.

Can you tell me how can I now test the deployment? https://preprod-api.prestashop-project.org/modules/8.2.0 still doesn't contain nothing. 🤷‍♂️

Test and deployment

Read https://github.com/PrestaShop/distribution-api?tab=readme-ov-file#workflow

  • when you open a PR (on the upstream repo, like this one) you can add the integration-deployment to deploy the code on INTEGRATION environment integration-api.prestashop-project.org
  • when the PR is merged, this will deploy the code of main on PREPROD environment preprod-api.prestashop-project.org
  • when we do a GitHub release, this will deploy the code of the release tag on PROD environment api.prestashop-project.org

Your code should now be available on integration-api.prestashop-project.org

Looking at integration for your PR

On https://integration-api.prestashop-project.org/ I can see that

About logic

10.0.0 // Added
9.0.1 // Added
9.1.0 // Added
8.1.5 // Added
8.2.0 // Added

I think you went too far 😁 I don't think we need 9.1.0 right now you're 1 year early.

@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 5, 2024

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

@Hlavtox Your tests do not cover the generation logic. Your tests do not verify that, if the "highest found version" is 8.1.4, then development versions 8.1.5, 8.2.0 et 9.0.0 will be added. This is what I insist on having covered.

Your tests simply verify the model PrestaShop logic which is basically "how to use implode" ^^

We want to put tests in place not only to catch bugs now but also to catch bugs in the future, when the next developer comes and modifies this logic, and breaks something.

Do you need help to run the tests?

@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 5, 2024

@matks Mathieu could you write the tests please? I don't want to spend more time with it. 🙏

@Hlavtox Hlavtox force-pushed the automatically-add-new-versions branch 2 times, most recently from 40decd0 to 0a1b146 Compare February 5, 2024 11:47
@Hlavtox Hlavtox force-pushed the automatically-add-new-versions branch from d21218f to 36d7818 Compare February 5, 2024 13:39
Copy link
Contributor

@matks matks 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

if ($prestashopVersion->getVersion() === $nextPatchVersion) {
$addPatchVersion = false;
// Let's initialize utilities to get us the highest versions
$versionUtils = new VersionUtils();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: we could have injected it into the__construct using DI

@matks matks merged commit e26965c into main Feb 8, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration-deployment To deploy the assets to the integration storage
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants