-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
// 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'; |
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.
that's a risky default
test.js
Outdated
@@ -50,6 +50,10 @@ if (ci) { | |||
t.is(branch, real_branch) | |||
} | |||
}) | |||
} else if (ci && platform === 'gitlab') { |
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.
would you not want to test any of the other things 🤔
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 👍 |
Hey @joshghent @siddharthkp I might get some-time this week to work or provide any support on it. 👍 |
Year after and still no merge? |
@HiddeRpl would you like to contribute your time to take this pull request through? |
@siddharthkp As I mentioned before I can work on this to take it to completion. |
I might not, cc @kuldeepkeshwar who takes care of this repo |
@swapnilmishra predefined_variables 👈 might be helpful |
@kuldeepkeshwar Thanks for the link. The 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 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. 👍 |
Just explaining the changes a bit more to give more context as the PR is quite old. So by default 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 |
just curious, which version of bundlesize are you running? |
I am using version |
If you know the file, you could check it on unpkg: https://unpkg.com/browse/bundlesize@0.18.0/ |
Yes I can see the warn message in https://unpkg.com/browse/bundlesize@0.18.0/src/api.js at line no. 15 |
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. |
The output of gitlab CI with the current version can be seen here https://gitlab.com/swapnilmishra/test-ci/-/jobs/348930852 |
@swapnilmishra IMO we should also capture other variables too. All the information provided by |
@kuldeepkeshwar Sure, I will add them. 👍 |
- Added repo,branch,commit_message,pull_request_number,sh,event,jobUrl
- Added repo,branch,commit_message,pull_request_number,sh,event,jobUrl
@kuldeepkeshwar |
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.
LGTM
@swapnilmishra thanks for pulling this off |
This PR is complementing the changes required for siddharthkp/bundlesize#19
gitlab
as one of theci
variable value.gitlab CI output is attached below. For the CI test, I had to mirror the forked repo from github => gitlab and vice versa.