-
Notifications
You must be signed in to change notification settings - Fork 0
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
Devops/fairspc 55 GitHub actions #122
Conversation
1b5655a
to
c71a122
Compare
hmm, so there will be no check on PR? It would be good to see if the packaging is not broken or e.g. there are no lint errors (it was there on travis build, although I see the build was passing even with linter errors...) |
9bb1f99
to
ebe5339
Compare
@ewelinagr good point, added linter and realized that it would be easier to understand what exactly fails if put all helm commands into separate steps, so you will find more steps. By the way, GH Actions fails even if there is a warning |
ebe5339
to
c03ee02
Compare
@@ -20,6 +20,7 @@ fairspaceKeycloak: | |||
cpu: "500m" | |||
memory: "2048Mi" | |||
postgresql: | |||
password: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to add empty password? Rather have a failing deployment, than default password ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only reason is to satisfy helm linter's complains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me see what else we can do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I've done it in a different way: it did not fail before my changes because of the default value in pipeline, but linter failed. I made it behaving the same adding brackets what allows to have nullable values. In this case, we will take the default value again if it is absent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, it does not improve, but at least it does not make it worse. I wouldn't spend time on that
So there is no easy way to have some build/action on every PR? |
@ewelinagr good point, it makes sense to know that something is incorrect before merge. Changed the event triggerning (now on any push) and the push now is conditional (for dev and releases branches only |
No description provided.