-
Notifications
You must be signed in to change notification settings - Fork 79
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
[CAD-691] GitHub actions migration #368
Conversation
✅ SAST ReportNo issues found. Good job! 💪 |
34d72a7
to
deaed50
Compare
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.
Minor comments regarding consistent use of env vars
@@ -42,6 +46,10 @@ Anyone with the appropriate credentials can "cut a release" of jackdaw using the | |||
|
|||
Steps 2 to 6 is essentially `git tag $version -m "$title\n\n$description" && git push --tags` | |||
|
|||
#### Snapshot release | |||
|
|||
Snapshot releases can be created by pushing a tag with the format `publish-snapshot-semver` |
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.
Does this mean anyone can publish a snapshot release with any version they want?
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.
Described slightly higher in the README, deploy env is protected, deploy step cannot run without approval from jackdaw or credit-applications team review.
In any case outside contributors don't have write access, PR's are opened via forks.
|
||
:git-version | ||
{:status-to-version | ||
(fn [{:keys [tag branch ahead? dirty?] :as git}] | ||
(if (and tag (not ahead?) (not dirty?)) | ||
(if (and tag (not ahead?) (not dirty?) (= "master" branch)) |
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.
Should this be handled with trigger filters in the workflow def?
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.
Added to make more explicit that all non-master builds are marked snapshot; it should be handled already by the current setup, happy to remove it if it adds confusion.
@@ -40,7 +40,7 @@ | |||
:key-serde :long | |||
:value-serde :json})) | |||
|
|||
(def kafka-config {"bootstrap.servers" "localhost:9092" | |||
(def kafka-config {"bootstrap.servers" "kafka:9092" |
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.
(def kafka-config {"bootstrap.servers" "kafka:9092" | |
(def kafka-config {"bootstrap.servers" (str (utils/bootstrap-servers) ":9092") |
:config {"bootstrap.servers" "kafka:9092" | ||
"application.id" app-id}}) |
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.
:config {"bootstrap.servers" "kafka:9092" | |
"application.id" app-id}}) | |
:config (merge kafka-config {"application.id" app-id})}) |
test/jackdaw/test_test.clj
Outdated
{"bootstrap.servers" (str (utils/bootstrap-servers) ":9092") | ||
"application.id" "test-echo-stream"}) |
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.
{"bootstrap.servers" (str (utils/bootstrap-servers) ":9092") | |
"application.id" "test-echo-stream"}) | |
(assoc kafka-config "application.id" "test-echo-stream") |
update other cases too
9ad0d45
to
f806698
Compare
f806698
to
88e227c
Compare
clojars_deploy
- running this step requires approval from a required approver (@FundingCircle/team-credit-application and @FundingCircle/jackdaw) currently