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

gitlab CI support #10

Merged
merged 17 commits into from
Nov 24, 2019
Merged

gitlab CI support #10

merged 17 commits into from
Nov 24, 2019

Conversation

swapnilmishra
Copy link
Contributor

@swapnilmishra swapnilmishra commented Nov 4, 2017

This PR is complementing the changes required for siddharthkp/bundlesize#19

  1. Add gitlab as one of the ci variable value.
  2. Tests for gitlab ci variable.
  3. Adds .gitlab-ci.yml to run the test in gitlab CI.

gitlab CI output is attached below. For the CI test, I had to mirror the forked repo from github => gitlab and vice versa.

image

// platform denotes code hosting provider i.e github, gitlab, bitbucket etc.
// Had to introduce this variable as there are cases when CI is run on the same platform where code is hosted as those cases need to be handled differently.
// Default value is github
let platform = 'github';
Copy link
Owner

Choose a reason for hiding this comment

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

that's a risky default

test.js Outdated
@@ -50,6 +50,10 @@ if (ci) {
t.is(branch, real_branch)
}
})
} else if (ci && platform === 'gitlab') {
Copy link
Owner

Choose a reason for hiding this comment

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

would you not want to test any of the other things 🤔

@joshghent
Copy link

Hey @siddharthkp, if I get this branch fixed up, could we get it merged so we can also merge siddharthkp/bundlesize#180?

Very keen to get bundlesize up and running at my company 👍

@swapnilmishra
Copy link
Contributor Author

swapnilmishra commented Oct 10, 2018

Hey @joshghent @siddharthkp I might get some-time this week to work or provide any support on it. 👍

@HiddeRpl
Copy link

Year after and still no merge?

@siddharthkp
Copy link
Owner

@HiddeRpl would you like to contribute your time to take this pull request through?

@swapnilmishra
Copy link
Contributor Author

@siddharthkp As I mentioned before I can work on this to take it to completion.
Would you be able to review the final set of changes then?
I will wait for your response before starting the work.

@siddharthkp
Copy link
Owner

I might not, cc @kuldeepkeshwar who takes care of this repo

@kuldeepkeshwar
Copy link
Collaborator

@swapnilmishra predefined_variables 👈 might be helpful

@swapnilmishra
Copy link
Contributor Author

@kuldeepkeshwar Thanks for the link. The GITLAB_CI variable that I am using to identify if the CI is from gitlab is from that only. Would you also like to use any other variable here?

What I feel is we don't need to use anything else for gitlab CI because the reporting happens within the platform itself based on the exit code. This is because in the case of gitlab, the CI is integrated into the platform itself. This means we don't need detail like repo, commit_message, sha etc. The status badge is also provided by gitlab based on the CI status.

But please let me know if you would like me to make more changes. I would be happy to do them in order to close the PR. 👍

@swapnilmishra
Copy link
Contributor Author

Just explaining the changes a bit more to give more context as the PR is quite old.

So by default bundlesize works fine on gitlab CI but it just throws this warning message which is confusing. The message which is shown is from this line from bundlesize codebase https://github.com/siddharthkp/bundlesize/blob/master/src/api.js#L14. See this image for the exact CI output.

At the moment the line which use to show warning(at the time of creating this PR) is commented out but whenever this change is reverted(mentioned in TODO) comment it is going to show the message again. So all we needed was a way to figure out that the bundlesize is running in gitlab CI and not show the warning.

@siddharthkp
Copy link
Owner

just curious, which version of bundlesize are you running?

@swapnilmishra
Copy link
Contributor Author

I am using version 0.18.0 which still prints the warning message. And I guess that is because the changes related to commenting the warning are not yet released but are there in master.

@siddharthkp
Copy link
Owner

If you know the file, you could check it on unpkg: https://unpkg.com/browse/bundlesize@0.18.0/

@swapnilmishra
Copy link
Contributor Author

Yes I can see the warn message in https://unpkg.com/browse/bundlesize@0.18.0/src/api.js at line no. 15

@swapnilmishra
Copy link
Contributor Author

swapnilmishra commented Nov 13, 2019

Are these changes(of master) in which the warn message is commented out(given here) is going to be part of the next release?

I am asking because if the warn message is permanently going to be removed then we don't even need this change.

@swapnilmishra
Copy link
Contributor Author

The output of gitlab CI with the current version can be seen here https://gitlab.com/swapnilmishra/test-ci/-/jobs/348930852

@kuldeepkeshwar
Copy link
Collaborator

kuldeepkeshwar commented Nov 16, 2019

@swapnilmishra IMO we should also capture other variables too.

All the information provided by ci-env is always present in the environment. ci-env just provides a uniformed way of accessing that information.

@swapnilmishra
Copy link
Contributor Author

@kuldeepkeshwar Sure, I will add them. 👍

Swapnil and others added 2 commits November 18, 2019 15:49
- Added repo,branch,commit_message,pull_request_number,sh,event,jobUrl
- Added repo,branch,commit_message,pull_request_number,sh,event,jobUrl
@swapnilmishra
Copy link
Contributor Author

@kuldeepkeshwar
Added all the CI variables for gitlab along with tests for each of them.
CI output can be viewed: https://gitlab.com/swapnilmishra/ci-env/-/jobs/354041324 and in the screenshot below.
Screenshot 2019-11-18 at 17 38 25

test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@kuldeepkeshwar kuldeepkeshwar left a comment

Choose a reason for hiding this comment

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

LGTM

@kuldeepkeshwar
Copy link
Collaborator

@swapnilmishra thanks for pulling this off

@kuldeepkeshwar kuldeepkeshwar merged commit 24db072 into siddharthkp:master Nov 24, 2019
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.

None yet

5 participants