feat(helm): Add Helm repo polling and caching #829
Conversation
|
@ethanfrogers reminder to review it whenever you can! |
|
hi @ezimanyi -- since this PR is pending for a while, do you know anyone else that can help with reviewing it? Thanks! |
ethanfrogers
requested review from
german-muzquiz
and removed request for
ethanfrogers
igor-web/igor-web.gradle
Outdated
| @@ -36,7 +36,8 @@ dependencies { | |||
| implementation "com.sun.xml.bind:jaxb-core:2.3.0.1" | |||
| implementation "com.sun.xml.bind:jaxb-impl:2.3.2" | |||
|
|
|||
| implementation "com.vdurmont:semver4j:3.1.0" | |||
| implementation "commons-io:commons-io" | |||
| implementation 'com.vdurmont:semver4j:3.1.0' | |||
There was a problem hiding this comment.
Seen this dependency introduced in multiple places. It should be added to kork/spinnaker-dependencies.
There was a problem hiding this comment.
Hmm, this change doesn't actually use the semver4j library - this change is importing commons-io. However, I think standardizing on a semver library is a good thing.
Created: spinnaker/kork#784
The related echo PR did use that same semver4j library, and I can refactor that as well once there's a kork build for the above.
| } | ||
|
|
||
| // Custom converter to deal with index file raw string responses | ||
| class StringConverter implements Converter { |
There was a problem hiding this comment.
Putting a class inside of the method seems like a bad move - maybe just make it an inner class of HelmConfig.
|
|
||
| @ConditionalOnProperty("helm.enabled") | ||
| @ConfigurationProperties(prefix = "helm") | ||
| public class HelmProperties {} |
| Optional<HelmAccount> account = | ||
| helmAccounts.accounts.stream().filter(it -> it.name.equals(partition)).findFirst(); | ||
| if (!account.isPresent()) { | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
Please use the SpinnakerException family.
There was a problem hiding this comment.
Will change to ConstraintViolationException
igor-web/src/main/java/com/netflix/spinnaker/igor/helm/HelmMonitor.java
Outdated
Show resolved
Hide resolved
igor-web/src/main/java/com/netflix/spinnaker/igor/helm/accounts/HelmAccount.java
Outdated
Show resolved
Hide resolved
igor-web/src/main/java/com/netflix/spinnaker/igor/helm/accounts/HelmAccounts.java
Outdated
Show resolved
Hide resolved
igor-web/src/main/java/com/netflix/spinnaker/igor/helm/accounts/HelmAccounts.java
Show resolved
Hide resolved
igor-web/src/main/java/com/netflix/spinnaker/igor/helm/HelmMonitor.java
Outdated
Show resolved
Hide resolved
igor-web/src/main/java/com/netflix/spinnaker/igor/helm/cache/HelmCache.java
Outdated
Show resolved
Hide resolved
igor-web/src/main/java/com/netflix/spinnaker/igor/helm/HelmMonitor.java
Outdated
Show resolved
Hide resolved
igor-web/src/main/java/com/netflix/spinnaker/igor/helm/model/HelmEntry.java
Outdated
Show resolved
Hide resolved
- Helm polling/caching/triggering - Models and etc. for Helm repository information
jcavanagh
force-pushed
the
helm-watcher-upstream
branch
from
3b0e7e5
to
4412b74
Compare
|
Thanks @robzienert and @german-muzquiz for the detailed feedback! I believe I have addressed everything. Note that this build will fail until spinnaker/kork#784 is merged and bumped here, as the |
|
@robzienert Thanks for merging the kork change! Let me know if there's anything else you'd like to see changed in this PR. |
|
hi @jcavanagh, I tried to update your branch so we can merge it but it failed. Can you please take a look so we can actually merge this? thanks! |
|
Weird build error, just going to retry it: |
|
@gal-yardeni Build is happy again 🎉 |
Relates to issue: spinnaker/spinnaker#4447
Related PRs:
spinnaker/echo#988
spinnaker/deck#8475
spinnaker/clouddriver#4773