-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Feat: Adding limit field to template and Taks, overriding the CLI args from task. #2420
Open
Gnyblast
wants to merge
14
commits into
semaphoreui:develop
Choose a base branch
from
Gnyblast:develop
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
383d72c
feat(TaskForm and Templates): Added hosts limit
8fabb11
Merge remote-tracking branch 'origin/develop' into develop
2cb4e59
refactor: added comment to explain code removal reason
b8a3adb
fix: Fixing the sql statement to work with both postgresql and mysql
0ab5688
Merge branch 'semaphoreui:develop' into develop
Gnyblast fa52f36
refactor: migration script handler for mysql & postges, also fixed dr…
Gnyblast e258996
fix: integration test failure for pq
Gnyblast fd2b13e
Merge remote-tracking branch 'real/develop' into develop
cd088c9
feat: Making cli override args detection
490d97e
fix: code analysis fix for sql migration
4792716
Merge branch 'develop' into develop
fiftin c9a3492
refactor: Fixing port mistakenly added to package.json and error text…
ca83807
fix(cliArgs): Fixing the CLI args bug for removed overrides
92664be
Merge remote-tracking branch 'real/develop' into develop
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
package sql | ||
|
||
import "github.com/go-gorp/gorp/v3" | ||
|
||
type migration_2_10_27 struct { | ||
db *SqlDb | ||
} | ||
|
||
func (m migration_2_10_27) PostApply(tx *gorp.Transaction) error { | ||
switch m.db.sql.Dialect.(type) { | ||
case gorp.MySQLDialect: | ||
_, _ = tx.Exec(m.db.PrepareQuery("alter table `task` modify `hosts_limit` text default null;")) | ||
case gorp.PostgresDialect: | ||
_, err := tx.Exec( | ||
m.db.PrepareQuery("alter table `task` alter column `hosts_limit` type text, alter column `hosts_limit` drop not null, alter column `hosts_limit` set default null;")) | ||
return err | ||
} | ||
return nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
alter table project__template add column hosts_limit text default null; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
package tasks | ||
|
||
import ( | ||
"testing" | ||
) | ||
|
||
func TestIsCLIArgsOverridden(t *testing.T) { | ||
var args []struct { | ||
tmplArg string | ||
taskArg string | ||
want bool | ||
err error | ||
} = []struct { | ||
tmplArg string | ||
taskArg string | ||
want bool | ||
err error | ||
}{ | ||
{ | ||
tmplArg: "--ssh-extra-args=\"-p 3222\"", | ||
taskArg: "--ssh-extra-args \"-p 3222\"", | ||
want: false, | ||
err: nil, | ||
}, | ||
{ | ||
tmplArg: "--ssh-extra-args=\"-p 3222\"", | ||
taskArg: "--ssh-extra-args \"-p 3223\"", | ||
want: true, | ||
err: nil, | ||
}, | ||
{ | ||
tmplArg: "--ssh-extra-args=\"-p 3222\"", | ||
taskArg: "--ssh-extra-args=\"-p 3223\"", | ||
want: true, | ||
err: nil, | ||
}, | ||
{ | ||
tmplArg: "--ssh-extra-args=\"-p 3222\"", | ||
taskArg: "--ssh-extra-args", | ||
want: false, | ||
err: errCliOverrideParseError, | ||
}, | ||
} | ||
|
||
for _, tc := range args { | ||
|
||
got, err := isCLIArgsOverridden(tc.tmplArg, tc.taskArg) | ||
if err != tc.err { | ||
t.Fail() | ||
} | ||
|
||
if got != tc.want { | ||
t.Fail() | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Single argument can not contain space which separates key and value. It must be 2 separate arguments. Otherwise
ansible-playbook
doesn't accept this argument (tested).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.
I didn't get full what you mean? Do you mean that
--ssh-extra-args
will only work if separator is=
sign? Like:-ssh-extra-args="-p 3222"
? and won't work with--ssh-extra-args "-p 3222"
? If that's the case, how do you prevent this on template CLI args declaration already? And this is not a user mistake if that's the case? On the other hand this piece of code only is to determine if they are passing same argument from task CLI args, so that it can be overriden, I don't really touch CLI args how they look like, regex is just checking either first=
or first white space and splits it from there, so it can compare the passed option,--ssh-extra-args
in this case.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.
@Gnyblast yes, it is true. Argument
--ssh-extra-args "-p 3222"
doesn't work in Semaphore.You can try to call ansible CLI with params
ansible-playbook '--ssh-extra-args "-p 3222"'
and you will got error. But following command will work:ansible-playbook '--ssh-extra-args' '-p 3222'
Semaphore runs ansible by command like
exec.Command("ansible-playbook", []string{"--ssh-extra-args \"-p 3222\""})
so it will not work. But following code will work:exec.Command("ansible-playbook", []string{"--ssh-extra-args", "-p 3222"})
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.
as I said before this piece of code here is just to test if the parser regex can split argument and it's value correctly by delimiter which is either
=
sign or a space. I don't do any formatting as you can see in the code below:first piece with
isCLIArgsOverridden
is the logic split them by delimiter and decide if it's same argument key with different value, the:1- if yes, that means it's an override and delete the one on
templateExtraArgs
sincetaskExtraArgs
will override that specific arguments2- If not and if there are duplicate, then again delete the on in
templateExtraArgs
sincetaskExtraArgs
already have a duplicate of it.So if user does a mistake on how to pass a CLI argument, which they can do it now with the current code, it's a user mistake and should be fixed by passing it correctly but using
=
sign or escaping correctly. Nothing changes there.