Skip to content

feat: add priorityclassname and lifecycle #313

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.

Sign up for GitHub

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

Jump to bottom
Merged
aslafy-z merged 4 commits into from
Jul 16, 2024
Merged

feat: add priorityclassname and lifecycle #313

aslafy-z merged 4 commits into from
Jul 16, 2024

Conversation

@rtest12
Copy link
Contributor

@rtest12 rtest12 commented May 7, 2024 •
edited
Loading

add priorityclassname and lifecycle

Screenshot from 2024-06-03 14-55-37
Screenshot from 2024-05-29 17-55-36
Untitled

@rtest12 rtest12 marked this pull request as draft May 7, 2024 11:40
@rtest12 rtest12 force-pushed the feature/add-priorityclassname-and-lifecycle branch from 93669a4 to 8bdf177 Compare May 28, 2024 18:56
@github-actions
Copy link

github-actions bot commented Jun 3, 2024

@rtest12 validation successful`

@rtest12 rtest12 force-pushed the feature/add-priorityclassname-and-lifecycle branch 2 times, most recently from ef0e245 to 4fc74c2 Compare June 5, 2024 07:27
@rtest12 rtest12 marked this pull request as ready for review June 5, 2024 07:28
@rtest12
Copy link
Contributor Author

rtest12 commented Jun 5, 2024

@AsfaMumtaz @rasheedamir
ptl

This was linked to issues Jun 5, 2024
add lifeCycle block to the deployment container #299
Closed
add priorityClassName for the deployment #298
Closed
@rasheedamir
Copy link
Member

rasheedamir commented Jun 23, 2024

@d3adb5 can you review it?

@rasheedamir rasheedamir requested a review from d3adb5 June 23, 2024 16:02
d3adb5
d3adb5 previously approved these changes Jun 23, 2024
View reviewed changes
Copy link
Collaborator

@d3adb5 d3adb5 left a comment

Choose a reason for hiding this comment

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

I left one suggestion, and otherwise this LGTM, however I want to note that the majority of these changes have to do with whitespace and formatting. While that kind of tidying up is welcome, I'd much prefer it to happen in its own commit. Can you perhaps separate them, @rtest12?

Could you chime in here, @rasheedamir?

@rtest12 rtest12 force-pushed the feature/add-priorityclassname-and-lifecycle branch from 4d4be33 to dc8254a Compare June 25, 2024 10:51
@rtest12
Copy link
Contributor Author

rtest12 commented Jun 25, 2024

I left one suggestion, and otherwise this LGTM, however I want to note that the majority of these changes have to do with whitespace and formatting. While that kind of tidying up is welcome, I'd much prefer it to happen in its own commit. Can you perhaps separate them, @rtest12?

@rasheedamir @d3adb5
Ok done, I've split the logic into two separate commits.

@rtest12 rtest12 requested a review from d3adb5 June 25, 2024 10:59
d3adb5
d3adb5 previously approved these changes Jun 25, 2024
View reviewed changes
Copy link
Collaborator

@d3adb5 d3adb5 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you, @rtest12.

@rtest12
Copy link
Contributor Author

rtest12 commented Jul 3, 2024

@rasheedamir Could you take a look and release this PR please?

@rtest12
Copy link
Contributor Author

rtest12 commented Jul 9, 2024

Any news?
@d3adb5 @hanzala1234 @mustafaStakater @usamaahmadkhan @kahootali @rasheedamir @aslafy-z @AsfaMumtaz @sabkat @KhizerJaan
Could you take a look and release this PR please?

aslafy-z
aslafy-z reviewed Jul 9, 2024
View reviewed changes
aslafy-z
aslafy-z reviewed Jul 9, 2024
View reviewed changes
aslafy-z
aslafy-z reviewed Jul 9, 2024
View reviewed changes
aslafy-z
aslafy-z reviewed Jul 9, 2024
View reviewed changes
aslafy-z
aslafy-z reviewed Jul 9, 2024
View reviewed changes
Copy link
Collaborator

@aslafy-z aslafy-z left a comment

Choose a reason for hiding this comment

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

Hey, thank you for your contrib. I added a few comments.

@rtest12
Copy link
Contributor Author

rtest12 commented Jul 10, 2024

Hey, thank you for your contrib. I added a few comments.

@aslafy-z @d3adb5
Done, please re-approve

@rtest12 rtest12 requested review from aslafy-z and d3adb5 July 10, 2024 07:53
aslafy-z
aslafy-z requested changes Jul 10, 2024
View reviewed changes
Copy link
Collaborator

@aslafy-z aslafy-z left a comment

Choose a reason for hiding this comment

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

Missing change that break ci

@rtest12 rtest12 requested a review from aslafy-z July 10, 2024 08:17
@rtest12
Copy link
Contributor Author

rtest12 commented Jul 10, 2024

Missing change that break ci

@aslafy-z
Done

@aslafy-z
Copy link
Collaborator

aslafy-z commented Jul 10, 2024 •
edited
Loading

This PR now hits the same issue as #320 (review), reported as #321.
Will ping stakater team so they fix it asap.
I'll approve and merge once the build passes.

@rtest12
Copy link
Contributor Author

rtest12 commented Jul 10, 2024

This PR now hits the same issue as #320 (review), reported as #321. Will ping stakater team so they fix it asap. I'll approve and merge once the build passes.

@aslafy-z
Well thank you

@aslafy-z
Copy link
Collaborator

aslafy-z commented Jul 12, 2024

@rtest12 CI is OK, can you please rebase your PR?

@rtest12 rtest12 force-pushed the feature/add-priorityclassname-and-lifecycle branch from a7bd3ec to 07dfb90 Compare July 12, 2024 14:26
@rtest12
Copy link
Contributor Author

rtest12 commented Jul 12, 2024

@rtest12 CI is OK, can you please rebase your PR?

@aslafy-z
Done, please check again

@aslafy-z
Copy link
Collaborator

aslafy-z commented Jul 12, 2024 •
edited
Loading

@rtest12 Helm lint fails with an error (you can safely ignore the warnings). Can you give a look please?

@rtest12
Copy link
Contributor Author

rtest12 commented Jul 12, 2024

@rtest12 Helm lint fails with an error (you can safely ignore the warnings). Can you give a look please?

@aslafy-z Could you attach a screenshot?

@aslafy-z
Copy link
Collaborator

aslafy-z commented Jul 12, 2024 •
edited
Loading

@rtest12 See #313 (comment)

@rtest12 @aslafy-z
Update application/templates/deployment.yaml
db99ded 
Co-authored-by: Zadkiel AHARONIAN <zadkiel.aharonian@gmail.com>
@rtest12
Copy link
Contributor Author

rtest12 commented Jul 12, 2024 •
edited
Loading

@rtest12 See #313 (comment)

@aslafy-z
Sure, done, this commit was lost after a rebase

aslafy-z
aslafy-z approved these changes Jul 12, 2024
View reviewed changes
Copy link
Collaborator

@aslafy-z aslafy-z left a comment

Choose a reason for hiding this comment

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

LGTM!

@rasheedamir
Copy link
Member

rasheedamir commented Jul 13, 2024

@d3adb5 can you approve it as well if all good? so, it can be merged?

d3adb5
d3adb5 approved these changes Jul 14, 2024
View reviewed changes
Copy link
Collaborator

@d3adb5 d3adb5 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! Thanks, @rtest12.

@aslafy-z aslafy-z merged commit ea6cd5b into stakater:master Jul 16, 2024
@rtest12 rtest12 mentioned this pull request Jul 16, 2024
Closed
@rtest12 rtest12 deleted the feature/add-priorityclassname-and-lifecycle branch August 9, 2024 20:35
@rtest12 rtest12 mentioned this pull request Aug 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aslafy-z aslafy-z aslafy-z approved these changes

@d3adb5 d3adb5 d3adb5 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add lifeCycle block to the deployment container add priorityClassName for the deployment

4 participants

@rtest12 @rasheedamir @aslafy-z @d3adb5