search

stakater / application

Generic helm chart for all kind of applications
http://stakater.com
MIT License
199 stars 85 forks source link

feat: add priorityclassname and lifecycle #313

Closed rtest12 closed 2 months ago

rtest12 commented 4 months ago

add priorityclassname and lifecycle

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

github-actions[bot] commented 3 months ago

@rtest12 validation successful`

rtest12 commented 3 months ago

@AsfaMumtaz @rasheedamir ptl

rasheedamir commented 2 months ago

@d3adb5 can you review it?

rtest12 commented 2 months ago

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 commented 2 months ago

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

rtest12 commented 2 months ago

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

rtest12 commented 2 months ago

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

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

rtest12 commented 2 months ago

Missing change that break ci

@aslafy-z Done

aslafy-z commented 2 months ago

This PR now hits the same issue as https://github.com/stakater/application/pull/320#pullrequestreview-2161390342, reported as #321. Will ping stakater team so they fix it asap. I'll approve and merge once the build passes.

rtest12 commented 2 months ago

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 commented 2 months ago

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

rtest12 commented 2 months ago

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

@aslafy-z Done, please check again

aslafy-z commented 2 months ago

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

rtest12 commented 2 months ago

@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 commented 2 months ago

@rtest12 See https://github.com/stakater/application/pull/313#discussion_r1671828812

rtest12 commented 2 months ago

@rtest12 See #313 (comment)

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

rasheedamir commented 2 months ago

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