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

Add support to print information about newly upgraded Jenkins #593

Merged
merged 15 commits into from
Sep 3, 2021

Conversation

jxr98
Copy link
Contributor

@jxr98 jxr98 commented Jul 18, 2021

Make sure that you've checked the boxes below before you submit PR:

Always

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
  • Written well with PR title, we generate the release notes base on that

For the bug fixes or features only

  • Quality Gate Passed. Change this URL to your PR.
  • The coverage is xxx on the new lines
  • I've tested it by manual in the following platform
    • MacOS
    • Linux
    • Windows
  • Unit Test covered
  • e2e Test covered

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2021

Codecov Report

Merging #593 (ac7a77e) into master (9bfb8da) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #593   +/-   ##
=======================================
  Coverage   37.98%   37.98%           
=======================================
  Files          13       13           
  Lines         416      416           
=======================================
  Hits          158      158           
  Misses        246      246           
  Partials       12       12           
Flag Coverage Δ
unittests 37.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 9bfb8da...ac7a77e. Read the comment docs.

err = ioutil.WriteFile(rootOptions.ConfigFile, data, 0664)
Expect(err).To(BeNil())

resp, _ := http.Get(LTSURL)
Copy link
Member

Choose a reason for hiding this comment

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

Any real network request should not be here. We cannot guarantee all environments have internet access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I am going to change this.

"strings"
)

const LTSURL = "https://www.jenkins.io/changelog-stable/rss.xml"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I have changed this part.

@jxr98 jxr98 changed the title Add support to print information about newly upgraded plugins Add support to print information about newly upgraded Jenkins Jul 19, 2021
err = ioutil.WriteFile(rootOptions.ConfigFile, data, 0664)
Expect(err).To(BeNil())

resp, _ := http.Get(LTSURL)
Copy link
Member

Choose a reason for hiding this comment

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

Where is the LTSURL value defined??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the center_list.go. The LTSURL is a constant and I changed the name from LTSURL to LtsUrl. But I haven't changed the test file. I will have it done in a few days

var centerListOption CenterListOption
xml.Unmarshal(bytes, &centerListOption)
theNewestJenkinsVersion := centerListOption.Channel.Items[0].Title[8:]
temp, _ := printChangelog(LTSURL, theNewestJenkinsVersion)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Member

@LinuxSuRen LinuxSuRen left a comment

Choose a reason for hiding this comment

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

Hi @jxr98 , thanks for your contribution. I found some small issues. Please take a look at it. And please don't forget to check the CI status from GitHub. The status of Lint was failed.

By the way, the e2e tests have a big chance to fail. So I suggest that we can skip them for now. See also https://github.com/jenkins-zh/jenkins-cli/blob/master/.github/workflows/e2e-test.yaml

@@ -1,6 +1,7 @@
package cmd

import (
bytes2 "bytes"
Copy link
Member

Choose a reason for hiding this comment

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

Why do you give it an alias bytes2?

</rss>`
}

func resultOneVersionData() string {
Copy link
Member

Choose a reason for hiding this comment

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

I don't block here. But a better way might be putting these strings into a file. Golang allows us to embed files into the binary file.

Refs

//the number of lines to be printed in description column
const NumberOfLinesOfDescription = 10
//ASCII of line feed
const AsciiOfLineFeed = 10
Copy link
Member

Choose a reason for hiding this comment

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

@jxr98 Please check the tips from hound.

//ASCII of line feed
const AsciiOfLineFeed = 10
//ASCII of space
const AsciiOfSpace = 32
Copy link
Member

Choose a reason for hiding this comment

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

@jxr98 Please check the tips from hound.

@jxr98
Copy link
Contributor Author

jxr98 commented Jul 25, 2021

The reason why the e2e test failed this time was the go version used by this project was 1.15, but "embed" was added in go 1.16. I am gonna find a solution.

@LinuxSuRen
Copy link
Member

failed

We can use go 1.16

@yJunS
Copy link
Member

yJunS commented Jul 28, 2021

The reason why the e2e test failed this time was the go version used by this project was 1.15, but "embed" was added in go 1.16. I am gonna find a solution.

If you are sure that go 1.16 version can be tested normally in e2e, we can update the test dependency version to 1.16.

@yJunS
Copy link
Member

yJunS commented Jul 29, 2021

#599 I already pull request to update e2e go version

@jxr98
Copy link
Contributor Author

jxr98 commented Aug 20, 2021

Hello @LinuxSuRen! Sorry to bother you. The e2e test failed again after I pulled from origin and resolved the conflict. The TestPowerShellCompletion failed and I have no idea what the reason is. Could you please give me a little hint on it? Thanks so much.

@LinuxSuRen
Copy link
Member

Hello @LinuxSuRen! Sorry to bother you. The e2e test failed again after I pulled from origin and resolved the conflict. The TestPowerShellCompletion failed and I have no idea what the reason is. Could you please give me a little hint on it? Thanks so much.

Let's forget about the e2e tests for now.

@LinuxSuRen LinuxSuRen requested a review from yJunS August 20, 2021 05:43
@jxr98
Copy link
Contributor Author

jxr98 commented Aug 21, 2021

Hello @LinuxSuRen! Sorry to bother you. The e2e test failed again after I pulled from origin and resolved the conflict. The TestPowerShellCompletion failed and I have no idea what the reason is. Could you please give me a little hint on it? Thanks so much.

Let's forget about the e2e tests for now.

All right.

@jxr98 jxr98 force-pushed the centerlist branch 2 times, most recently from c1108ec to b50df06 Compare August 30, 2021 13:40
@LinuxSuRen
Copy link
Member

It would be better if you can tell us how to test this PR.

Copy link
Member

@LinuxSuRen LinuxSuRen left a comment

Choose a reason for hiding this comment

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

I feel confused about the following output. Maybe for many users, they just don't know what they should do next. So, it would be great to have more information about your CLI:

gitpod /workspace/jenkins-cli $ ./jcli center list
Error: Get "http://localhost:8080/jenkins/api/json": dial tcp [::1]:8080: connect: connection refused

@jxr98
Copy link
Contributor Author

jxr98 commented Aug 30, 2021

I feel confused about the following output. Maybe for many users, they just don't know what they should do next. So, it would be great to have more information about your CLI:

gitpod /workspace/jenkins-cli $ ./jcli center list
Error: Get "http://localhost:8080/jenkins/api/json": dial tcp [::1]:8080: connect: connection refused

I will figure this out tomorrow.

@LinuxSuRen LinuxSuRen merged commit 8f2c9f0 into jenkins-zh:master Sep 3, 2021
jxr98 added a commit to jxr98/jenkins-cli that referenced this pull request Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants