Skip to content
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

fix(archive): upsert archive + ci: Pin images on CI, add readiness probes, clean-up logging and other tweaks #2038

Merged
merged 7 commits into from
Jan 23, 2020

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Jan 22, 2020

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
  • Optional. I've added My organization is added to the README.
  • I've signed the CLA and required builds are green.

@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@1a96007). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #2038   +/-   ##
========================================
  Coverage          ?   8.78%           
========================================
  Files             ?      61           
  Lines             ?   34575           
  Branches          ?       0           
========================================
  Hits              ?    3036           
  Misses            ?   31148           
  Partials          ?     391
Impacted Files Coverage Δ
persist/sqldb/workflow_archive.go 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a96007...1c0bc2f. Read the comment docs.

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
@@ -69,7 +69,33 @@ func (r *workflowArchive) ArchiveWorkflow(wf *wfv1.Workflow) error {
},
Workflow: string(workflow),
})
return err
if err != nil {
if isDuplicateKeyError(err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is an actual code change

@@ -73,13 +73,25 @@ func (s *E2ESuite) TearDownSuite() {
func (s *E2ESuite) BeforeTest(_, _ string) {
s.Diagnostics = &Diagnostics{}

// delete all cron workflows
cronList, err := s.cronClient.List(metav1.ListOptions{LabelSelector: label})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let deletes crons before workflows, in case one get scheduled

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch here

@alexec alexec changed the title ci: Pin images on CI, add readiness probes fix(archive): try to upset archive + ci: Pin images on CI, add readiness probes, clean-up logging and other tweaks Jan 23, 2020
@@ -63,6 +62,12 @@ func (g *Given) Workflow(text string) *Given {
g.t.Fatal(err)
}
}
if g.wf.GetLabels() == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not all wfs were getting labelled - this fixes that

@@ -566,7 +566,7 @@ func (s *ArgoServerSuite) TestArtifactServer() {
Expect().
Status(200).
Body().
Contains("😀 Hello Argo!")
Contains(":) Hello Argo!")
Copy link
Contributor Author

@alexec alexec Jan 23, 2020

Choose a reason for hiding this comment

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

MySQL does not appear to have good emoticon support ☹️

@alexec alexec changed the title fix(archive): try to upset archive + ci: Pin images on CI, add readiness probes, clean-up logging and other tweaks fix(archive): upsert archive + ci: Pin images on CI, add readiness probes, clean-up logging and other tweaks Jan 23, 2020
@@ -73,13 +73,25 @@ func (s *E2ESuite) TearDownSuite() {
func (s *E2ESuite) BeforeTest(_, _ string) {
s.Diagnostics = &Diagnostics{}

// delete all cron workflows
cronList, err := s.cronClient.List(metav1.ListOptions{LabelSelector: label})
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch here

@@ -48,7 +48,7 @@ func (s *SmokeSuite) TestWorkflowTemplateBasic() {
When().
CreateWorkflowTemplates().
SubmitWorkflow().
WaitForWorkflow(30 * time.Second).
WaitForWorkflow(60 * time.Second).
Copy link
Member

Choose a reason for hiding this comment

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

If we're increasing the time here we should make sure we are also increasing it on the Makefile (go test ...)

@alexec alexec merged commit 1db74e1 into argoproj:master Jan 23, 2020
@alexec alexec deleted the pin-images branch January 23, 2020 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants