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

feat(job): deploy other version for jobs and advanced settings #435

Merged
merged 11 commits into from Jan 2, 2023

Conversation

bdebon
Copy link
Contributor

@bdebon bdebon commented Dec 29, 2022

What does this PR do?

  • Deploy another version for job with git
  • Deploy another version for job with container
  • Deploy another version for container
  • Advanced Settings

Link to the JIRA ticket
https://qovery.atlassian.net/jira/software/projects/FRT/boards/23?selectedIssue=FRT-626
https://qovery.atlassian.net/jira/software/projects/FRT/boards/23?selectedIssue=FRT-629

CleanShot 2022-12-29 at 17 36 09@2x

Put screenshot following the template before => after here


PR Checklist

Global

  • This PR does not introduce any breaking change
  • This PR introduces breaking change(s) and has been labeled as such
  • I have found someone to review this PR and pinged him

Store

  • This PR introduces new store changes

NX

  • I have run the dep-graph locally and made sure the tree was clean i.e no circular dependencies
  • I have followed the library pattern i.e feature, ui, data, utils

Clean Code

  • I made sure the code is type safe (no any)
  • I have included a feature flag on my feature, if applicable

@evoxmusic
Copy link
Contributor

A preview environment was automatically created via Qovery.
Click on the link below to follow its deployment and use it.
👉 [PR] staging - feat(job): deploy other version for jobs and advanced settings - 2022-12-29T16:36:01Z

Another comment will be posted when deployments are terminated

@nx-cloud
Copy link

nx-cloud bot commented Dec 29, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit eeff0fa. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@evoxmusic
Copy link
Contributor

Your preview environment has been successfully deployed !
Click on the link below to open your service:
👉 console
👉 storybook

Copy link
Member

@RemiBonnet RemiBonnet left a comment

Choose a reason for hiding this comment

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

Thanks, @bdebon, here are my suggestions to improve it :

  • I think we use too much as GitApplicationEntity, are you sure about it? I have the feeling the code is more complex and not clear like this. I think we can simplify it, the usage of "as" is ok in some cases but you have to be careful not to use it a lot

  • Why we indicated the service name here? If it's necessary I think we can add it directly on the paragraph

Capture d’écran 2022-12-29 à 18 03 58

Capture d’écran 2022-12-29 à 18 04 15

@bdebon
Copy link
Contributor Author

bdebon commented Dec 29, 2022

  • I don't think there is too much as. Job has introduced a lot of complexity in the overall typing of the project. We decided to consider it as an Application but it's in the end a weird application than can be either an application or a container and also an actual job with its own specificities. It adds some complexity and some extra typing. I don't think there is such a thing as using too many as. I used it when it's needed. I will change the only place I can use your other writing for that but all in all we can't reduce so much the as usage unfortunately. And I agree, it makes the code more verbose and harder to read but I don't think we have options here... Feel free to dig if you have time.

  • For the application name repeated in the modal, it's a request from Alessandro and something that we're gonna have to implement for almost all modals.
    It's not exactly the final design of this though. But we decided with Alessandro to wait a little bit before implementing the design with Icons.

CleanShot 2022-12-29 at 18 27 56@2x

@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2022

Codecov Report

Merging #435 (eeff0fa) into staging (facc3bf) will decrease coverage by 0.00%.
The diff coverage is 42.55%.

@@             Coverage Diff             @@
##           staging     #435      +/-   ##
===========================================
- Coverage    52.82%   52.81%   -0.01%     
===========================================
  Files          333      335       +2     
  Lines         6357     6413      +56     
  Branches      1412     1433      +21     
===========================================
+ Hits          3358     3387      +29     
- Misses        2539     2564      +25     
- Partials       460      462       +2     
Impacted Files Coverage Δ
.../application/src/lib/slices/application.actions.ts 0.00% <0.00%> (ø)
...re/page-settings-feature/page-settings-feature.tsx 0.00% <0.00%> (ø)
...re/storage-modal-feature/storage-modal-feature.tsx 61.53% <ø> (ø)
...ibs/pages/application/src/lib/page-application.tsx 61.90% <0.00%> (ø)
...bout-content-container/about-content-container.tsx 66.66% <0.00%> (ø)
.../page-settings-advanced/page-settings-advanced.tsx 92.85% <ø> (ø)
...on-buttons-actions/application-buttons-actions.tsx 53.22% <0.00%> (-4.67%) ⬇️
...s/application/src/lib/slices/applications.slice.ts 17.79% <10.71%> (-0.43%) ⬇️
...-general-feature/page-settings-general-feature.tsx 37.23% <15.78%> (+0.77%) ⬆️
...ure/page-settings-preview-environments-feature.tsx 43.24% <33.33%> (ø)
... and 16 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bdebon
Copy link
Contributor Author

bdebon commented Jan 2, 2023

I finally was able to remove a lot of as by implementing a full refactoring of the app by declaring the interface ApplicationEntity way differently. Before it was a type that was keeping only the property that were common to each of the property. We changed this idea for this approach:

export interface ApplicationEntity
  extends Partial<ContainerApplicationEntity>,
    Omit<Partial<GitApplicationEntity>, 'description' | 'default_advanced_settings' | 'advanced_settings'>,
    Partial<JobApplicationEntity> {
  id: string
  created_at: string
  name: string
}

It is a little bit more complex to declare but becomes much closer to the abstraction we decided which is to consider Job, Containers and GitApplication as an ApplicationEntity object. It drastically reduced the need for casting everywhere.
I had to move and adapt many files but it's a good win for the future! I'm super happy with this solution, it something I had in mind for a long time now but I did not know how to write it well and was a little bit overwhelmed by the refactoring it was inducing.

@bdebon bdebon merged commit 72e6a10 into staging Jan 2, 2023
@bdebon bdebon deleted the feat/job-deploy-another branch January 2, 2023 12:16
@bdebon
Copy link
Contributor Author

bdebon commented Jan 4, 2023

🎉 This PR is included in version 1.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@bdebon bdebon added the released label Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants