Logic simplifications #47
Conversation
1) Nested IF statements can be combined
2) Add/remove shellcmd contains logic that only needs to run if the early shellcmd array does/doesn't exist
3) Logs should be non-vague ("...added/removed a shellcmd" is better if it states the command added/removed, and for removal, clarify it removed an existing command)
Resubmitted as originally against old pfsense-packages, see pfsense/pfsense-packages#1229
|
Looks good, thanks! One last thing: |
|
Every single PR on the packages, however small, will need those 3 files updated? Please excuse any ignorance displayed by this question, but - can that be either automated, or only updated when there's something substantial and new? Otherwise it sounds like it could become a source of chaos for versioning, avoidable tripling of work for small PRs, or constant "new versions" for end users... :) |
|
It was like this in the past because we were migrating from pfSense-packages repo using a script to avoid maintain 2 different packages base. Now that we stopped sync'ing both repos it can be done using ports mechanisms. I'll work on a batch conversion to make it possible to set portversion on xml files based on main Makefile |
|
Regarding the second point, if version doesn't change, a new package will never be built. Builder is automated and track package versions. If you change package and doesn't change version you risky to have different software installed on customers. We can always use PORTREVISION for small changes and PORTVERSION changes for big ones |
|
Then shall I leave this for now? (It's uncharted waters for me, being unclear what line/s needs changing in Makefile, and sounds like script will take care of the other 2). As an aside on this specific PR it's small enough that I wonder if it can be left until the next "new version" someone does, and included as part of that. I am also a bit mindful as an end-user of the time + annoyance that could be caused if users arrived every day to find new reinstalls needed of their packages because some minor change took place in the source. Maybe for minor changes that can wait and be "rolled up" with others, it would be worth doing so? After all, it doesn't create a new firmware version for every 'core code' PR, either, even if PRs on the core router code are often quite significant. I've seen code with "every small update is a new version" and it can get pretty annoying if not done in a way that doesn't intrude on the end user/s, but presumably this would? |
|
If it's a non-functional change it can be merged without need to change version, then when a functional change is made, users will get a new version. |
|
It's fairly minor so that's OK, though I wouldn't want to get in the habit of letting things like this sit over time. If there is a problem it wouldn't be found until much later when it gets deployed. |
|
Yeah, @jim-p has a good point. If the non-functional change breaks something, we will just notice that in the future, and it's not a good practice. I'm going to commit a cleanup of info.xml and other internal xml files to make life easier so only PORTVERSION and/or PORTREVISION variables will need to be changed on main Makefile |
|
Done. Now only PORTREVISION or PORTVERSION need to be changed |
|
Much of this is a bit over my head :) But the fundamentals seem easy enough on a "monkey see, monkey do" basis. So where does this leave things for this PR, what do I need to know for future PRs to this repo, which are the fields (in which files) if any that need manually changing and the requirements for how and when they should change and the schema they will follow |
Presumably the number of occurances removed should always be 0 or 1, but if it's ever more then odd behaviour could be hard to track down, especially as all we match on is the fragment "apply_patches.php which could match other commands unwittingly in future. So this logs how many removals were done (even though should be redundant) because it won't hurt to be precise with an actual count.
|
(okay the >0 is strictly redundant but makes clear the outcome) |
| @@ -200,36 +195,35 @@ function bootup_apply_patches() { | |||
| function patch_add_shellcmd() { | |||
| global $config; | |||
| $a_earlyshellcmd = &$config['system']['earlyshellcmd']; | |||
There was a problem hiding this comment.
It must check if isset() or is_array() on $config['system']['earlyshellcmd'] before set the reference here. If it's not an array then it must be initialized as
if (!isset($config['system']['earlyshellcmd'])) {
$config['system']['earlyshellcmd'] = array();
}
$a_earlyshellcmd = &$config['system']['earlyshellcmd'];
After that , the if(is_array()) from line 199 can go away
|
Done - but please advise on my Q's a couple or so posts up, thankyou! |
| foreach ($a_earlyshellcmd as $idx => $cmd) { | ||
| if (stristr($cmd, "apply_patches.php")) { | ||
| $found = true; | ||
| if (is_array($a_earlyshellcmd)) { |
There was a problem hiding this comment.
This check is not necessary anymore, you are always setting it above
* Fix a build failure when building without YAJL support (#47, #49). * dnsqr: Also perform query name filtering for UDP_UNSOLICITED_RESPONSE messages (#48). * dnsqr: Remove 'icmp' from the generated BPF (#20, #50). * dnsqr: Only set 'resolver_address_zeroed' field if addresses were zeroed from the underlying query/response packet fields (#51). Resolver address zeroing only works for the UDP message types, so we were incorrectly setting the 'resolver_address_zeroed' field for TCP and ICMP messages. * nmsg-dnsqr2pcap: Also dump ICMP and TCP packets (#52). -- Robert Edmonds <edmonds@fsi.io> Fri, 29 Apr 2016 13:37:40 -0400 Sponsored by: Farsight Security, Inc.
|
@stilez also, please bump PORTVERSION on port Makefile to make sure a new package is built |
... but check they have the right type just in case!
|
Resubmitted as PR #199 to avoid conflicts |
v2.2.6: - Fix tests wrt double-quoting in provisioning URIs v2.2.5: - Quote issuer QS parameter in provisioning_uri. Fixes #47. - Raise an exception if a negative integer is passed to at() (#41). - Documentation and release infrastructure improvements. PR: 220400 Submitted by: Vladimir Krstulja <vlad-fbsd@acheronmedia.com> (maintainer) Approved by: garga (mentor, implicit)
Resubmit, as originally against old pfsense-packages, see pfsense/pfsense-packages#1229